tmpfs uiomove() conversion

[Martin Natano]

Below a diff to convert tmpfs/tmpfs_{subr,vnops}.c. That's an easy one.
Nearly all the relevant size variables already are a size_t, or a
smaller unsigned type. In the last hunk tn_size (an off_t) might be
passed to uiomove(), which is not problematic, because tn_size is
constrained to only hold values which are also representable by a
size_t.

cheers,
natano

Index: tmpfs_subr.c
===================================================================
RCS file: /cvs/src/sys/tmpfs/tmpfs_subr.c,v
retrieving revision 1.14
diff -u -p -u -r1.14 tmpfs_subr.c
--- tmpfs_subr.c 17 Apr 2015 04:43:21 -0000 1.14
+++ tmpfs_subr.c 1 Jan 2016 15:47:39 -0000
@@ -740,7 +740,7 @@ tmpfs_dir_getdotents(tmpfs_node_t *node,
return EJUSTRETURN;
}

- if ((error = uiomovei(dp, dp->d_reclen, uio)) != 0) {
+ if ((error = uiomove(dp, dp->d_reclen, uio)) != 0) {
return error;
}

@@ -837,7 +837,7 @@ tmpfs_dir_getdents(tmpfs_node_t *node, s
}

/* Copy out the directory entry and continue. */
- error = uiomovei(&dent, dent.d_reclen, uio);
+ error = uiomove(&dent, dent.d_reclen, uio);
if (error) {
break;
}
@@ -1225,7 +1225,7 @@ tmpfs_uiomove(tmpfs_node_t *node, struct
if (pgoff + len < PAGE_SIZE) {
va = tmpfs_uio_lookup(node, pgnum);
if (va != (vaddr_t)NULL)
- return uiomovei((void *)va + pgoff, len, uio);
+ return uiomove((void *)va + pgoff, len, uio);
}

if (len >= TMPFS_UIO_MAXBYTES) {
@@ -1249,7 +1249,7 @@ tmpfs_uiomove(tmpfs_node_t *node, struct
return error;
}

- error = uiomovei((void *)va + pgoff, sz, uio);
+ error = uiomove((void *)va + pgoff, sz, uio);
if (error == 0 && pgoff + sz < PAGE_SIZE)
tmpfs_uio_cache(node, pgnum, va);
else
Index: tmpfs_vnops.c
===================================================================
RCS file: /cvs/src/sys/tmpfs/tmpfs_vnops.c,v
retrieving revision 1.23
diff -u -p -u -r1.23 tmpfs_vnops.c
--- tmpfs_vnops.c 8 Dec 2015 15:26:25 -0000 1.23
+++ tmpfs_vnops.c 1 Jan 2016 15:47:52 -0000
@@ -1017,7 +1017,7 @@ tmpfs_readlink(void *v)
KASSERT(vp->v_type == VLNK);

node = VP_TO_TMPFS_NODE(vp);
- error = uiomovei(node->tn_spec.tn_lnk.tn_link,
+ error = uiomove(node->tn_spec.tn_lnk.tn_link,
MIN(node->tn_size, uio->uio_resid), uio);
tmpfs_update(node, TMPFS_NODE_ACCESSED);

[Stefan Kempf]

Post by Martin Natano
Below a diff to convert tmpfs/tmpfs_{subr,vnops}.c. That's an easy one.
Nearly all the relevant size variables already are a size_t, or a
smaller unsigned type. In the last hunk tn_size (an off_t) might be
passed to uiomove(), which is not problematic, because tn_size is
constrained to only hold values which are also representable by a
size_t.

Diff reads good to me and reviewed myself that tn_size is always >= 0.
I'm a bit uneasy though with passing signed values as-is to uiomove().
Can we somehow make it explicit that we know that the uiomove() argument is

Post by Martin Natano
= 0?

Changing types all over the place would be too much churn though.

I'm leaning towards an explicit size_t cast for signed size arguments as
an annotation, e.g.
uiomove(buf, (size_t)signed_value, uio);

And a check in uiomove(). If a negative value gets passed in by
accident, it will be caught.

Thoughts?

Index: kern_subr.c
===================================================================
RCS file: /cvs/src/sys/kern/kern_subr.c,v
retrieving revision 1.45
diff -u -p -U10 -r1.45 kern_subr.c
--- kern_subr.c 11 Dec 2015 16:07:02 -0000 1.45
+++ kern_subr.c 17 Jan 2016 10:56:11 -0000
@@ -46,20 +46,23 @@
#include <sys/resourcevar.h>

int
uiomove(void *cp, size_t n, struct uio *uio)
{
struct iovec *iov;
size_t cnt;
int error = 0;
struct proc *p;

+ if (n > SSIZE_MAX)
+ return (EINVAL);
+
p = uio->uio_procp;

#ifdef DIAGNOSTIC
if (uio->uio_rw != UIO_READ && uio->uio_rw != UIO_WRITE)
panic("uiomove: mode");
if (uio->uio_segflg == UIO_USERSPACE && p != curproc)
panic("uiomove: proc");
#endif
while (n > 0 && uio->uio_resid) {
iov = uio->uio_iov;

Post by Martin Natano
cheers,
natano
Index: tmpfs_subr.c
===================================================================
RCS file: /cvs/src/sys/tmpfs/tmpfs_subr.c,v
retrieving revision 1.14
diff -u -p -u -r1.14 tmpfs_subr.c
--- tmpfs_subr.c 17 Apr 2015 04:43:21 -0000 1.14
+++ tmpfs_subr.c 1 Jan 2016 15:47:39 -0000
@@ -740,7 +740,7 @@ tmpfs_dir_getdotents(tmpfs_node_t *node,
return EJUSTRETURN;
}
- if ((error = uiomovei(dp, dp->d_reclen, uio)) != 0) {
+ if ((error = uiomove(dp, dp->d_reclen, uio)) != 0) {
return error;
}
@@ -837,7 +837,7 @@ tmpfs_dir_getdents(tmpfs_node_t *node, s
}
/* Copy out the directory entry and continue. */
- error = uiomovei(&dent, dent.d_reclen, uio);
+ error = uiomove(&dent, dent.d_reclen, uio);
if (error) {
break;
}
@@ -1225,7 +1225,7 @@ tmpfs_uiomove(tmpfs_node_t *node, struct
if (pgoff + len < PAGE_SIZE) {
va = tmpfs_uio_lookup(node, pgnum);
if (va != (vaddr_t)NULL)
- return uiomovei((void *)va + pgoff, len, uio);
+ return uiomove((void *)va + pgoff, len, uio);
}
if (len >= TMPFS_UIO_MAXBYTES) {
@@ -1249,7 +1249,7 @@ tmpfs_uiomove(tmpfs_node_t *node, struct
return error;
}
- error = uiomovei((void *)va + pgoff, sz, uio);
+ error = uiomove((void *)va + pgoff, sz, uio);
if (error == 0 && pgoff + sz < PAGE_SIZE)
tmpfs_uio_cache(node, pgnum, va);
else
Index: tmpfs_vnops.c
===================================================================
RCS file: /cvs/src/sys/tmpfs/tmpfs_vnops.c,v
retrieving revision 1.23
diff -u -p -u -r1.23 tmpfs_vnops.c
--- tmpfs_vnops.c 8 Dec 2015 15:26:25 -0000 1.23
+++ tmpfs_vnops.c 1 Jan 2016 15:47:52 -0000
@@ -1017,7 +1017,7 @@ tmpfs_readlink(void *v)
KASSERT(vp->v_type == VLNK);
node = VP_TO_TMPFS_NODE(vp);
- error = uiomovei(node->tn_spec.tn_lnk.tn_link,
+ error = uiomove(node->tn_spec.tn_lnk.tn_link,
MIN(node->tn_size, uio->uio_resid), uio);
tmpfs_update(node, TMPFS_NODE_ACCESSED);

[Martin Natano]

Post by Stefan Kempf
I'm a bit uneasy though with passing signed values as-is to uiomove().
Can we somehow make it explicit that we know that the uiomove() argument is

= 0?

Changing types all over the place would be too much churn though.
I'm leaning towards an explicit size_t cast for signed size arguments as
an annotation, e.g.
uiomove(buf, (size_t)signed_value, uio);

I agree, a cast like that would make the intent clear to the reader. See
the regenerated diff below.

Post by Stefan Kempf
And a check in uiomove(). If a negative value gets passed in by
accident, it will be caught.
Thoughts?
Index: kern_subr.c
===================================================================
RCS file: /cvs/src/sys/kern/kern_subr.c,v
retrieving revision 1.45
diff -u -p -U10 -r1.45 kern_subr.c
--- kern_subr.c 11 Dec 2015 16:07:02 -0000 1.45
+++ kern_subr.c 17 Jan 2016 10:56:11 -0000
@@ -46,20 +46,23 @@
#include <sys/resourcevar.h>
int
uiomove(void *cp, size_t n, struct uio *uio)
{
struct iovec *iov;
size_t cnt;
int error = 0;
struct proc *p;
+ if (n > SSIZE_MAX)
+ return (EINVAL);
+
p = uio->uio_procp;
#ifdef DIAGNOSTIC
if (uio->uio_rw != UIO_READ && uio->uio_rw != UIO_WRITE)
panic("uiomove: mode");
if (uio->uio_segflg == UIO_USERSPACE && p != curproc)
panic("uiomove: proc");
#endif
while (n > 0 && uio->uio_resid) {
iov = uio->uio_iov;

I don't think this will fix the underlying problem. It is not possible
to detect all types of overflow inside of uiomove(). Let's take a look
at our tmpfs example: An off_t value is passed to uimove(). On i386,
size_t is an unsigned 32 bit integer and off_t is a signed 64 bit
integer. When the off_t value is truncated on conversion, the
'if (n > SSIZE_MAX)' condition might trigger, or not, depending on the
exact value of the original off_t value, resulting in unreliable
behaviour.

Unfortunately, I think the best we can do is to audit and fix all
uiomove() callers, which we already do for the purpose of the
conversion.

cheers,
natano


Index: tmpfs_subr.c
===================================================================
RCS file: /cvs/src/sys/tmpfs/tmpfs_subr.c,v
retrieving revision 1.14
diff -u -p -u -r1.14 tmpfs_subr.c
--- tmpfs_subr.c 17 Apr 2015 04:43:21 -0000 1.14
+++ tmpfs_subr.c 17 Jan 2016 12:05:59 -0000
@@ -740,7 +740,7 @@ tmpfs_dir_getdotents(tmpfs_node_t *node,
return EJUSTRETURN;
}

- if ((error = uiomovei(dp, dp->d_reclen, uio)) != 0) {
+ if ((error = uiomove(dp, dp->d_reclen, uio)) != 0) {
return error;
}

@@ -837,7 +837,7 @@ tmpfs_dir_getdents(tmpfs_node_t *node, s
}

/* Copy out the directory entry and continue. */
- error = uiomovei(&dent, dent.d_reclen, uio);
+ error = uiomove(&dent, dent.d_reclen, uio);
if (error) {
break;
}
@@ -1225,7 +1225,7 @@ tmpfs_uiomove(tmpfs_node_t *node, struct
if (pgoff + len < PAGE_SIZE) {
va = tmpfs_uio_lookup(node, pgnum);
if (va != (vaddr_t)NULL)
- return uiomovei((void *)va + pgoff, len, uio);
+ return uiomove((void *)va + pgoff, len, uio);
}

if (len >= TMPFS_UIO_MAXBYTES) {
@@ -1249,7 +1249,7 @@ tmpfs_uiomove(tmpfs_node_t *node, struct
return error;
}

- error = uiomovei((void *)va + pgoff, sz, uio);
+ error = uiomove((void *)va + pgoff, sz, uio);
if (error == 0 && pgoff + sz < PAGE_SIZE)
tmpfs_uio_cache(node, pgnum, va);
else
Index: tmpfs_vnops.c
===================================================================
RCS file: /cvs/src/sys/tmpfs/tmpfs_vnops.c,v
retrieving revision 1.23
diff -u -p -u -r1.23 tmpfs_vnops.c
--- tmpfs_vnops.c 8 Dec 2015 15:26:25 -0000 1.23
+++ tmpfs_vnops.c 17 Jan 2016 12:06:12 -0000
@@ -1017,8 +1017,8 @@ tmpfs_readlink(void *v)
KASSERT(vp->v_type == VLNK);

node = VP_TO_TMPFS_NODE(vp);
- error = uiomovei(node->tn_spec.tn_lnk.tn_link,
- MIN(node->tn_size, uio->uio_resid), uio);
+ error = uiomove(node->tn_spec.tn_lnk.tn_link,
+ MIN((size_t)node->tn_size, uio->uio_resid), uio);
tmpfs_update(node, TMPFS_NODE_ACCESSED);

return error;

[Stefan Kempf]

Post by Martin Natano

Post by Stefan Kempf
I'm a bit uneasy though with passing signed values as-is to uiomove().
Can we somehow make it explicit that we know that the uiomove() argument is

= 0?

Changing types all over the place would be too much churn though.
I'm leaning towards an explicit size_t cast for signed size arguments as
an annotation, e.g.
uiomove(buf, (size_t)signed_value, uio);

I agree, a cast like that would make the intent clear to the reader. See
the regenerated diff below.

Post by Stefan Kempf
And a check in uiomove(). If a negative value gets passed in by
accident, it will be caught.
Thoughts?
Index: kern_subr.c
===================================================================
RCS file: /cvs/src/sys/kern/kern_subr.c,v
retrieving revision 1.45
diff -u -p -U10 -r1.45 kern_subr.c
--- kern_subr.c 11 Dec 2015 16:07:02 -0000 1.45
+++ kern_subr.c 17 Jan 2016 10:56:11 -0000
@@ -46,20 +46,23 @@
#include <sys/resourcevar.h>
int
uiomove(void *cp, size_t n, struct uio *uio)
{
struct iovec *iov;
size_t cnt;
int error = 0;
struct proc *p;
+ if (n > SSIZE_MAX)
+ return (EINVAL);
+
p = uio->uio_procp;
#ifdef DIAGNOSTIC
if (uio->uio_rw != UIO_READ && uio->uio_rw != UIO_WRITE)
panic("uiomove: mode");
if (uio->uio_segflg == UIO_USERSPACE && p != curproc)
panic("uiomove: proc");
#endif
while (n > 0 && uio->uio_resid) {
iov = uio->uio_iov;

I don't think this will fix the underlying problem. It is not possible
to detect all types of overflow inside of uiomove(). Let's take a look
at our tmpfs example: An off_t value is passed to uimove(). On i386,
size_t is an unsigned 32 bit integer and off_t is a signed 64 bit
integer. When the off_t value is truncated on conversion, the
'if (n > SSIZE_MAX)' condition might trigger, or not, depending on the
exact value of the original off_t value, resulting in unreliable
behaviour.

D'oh. Yes, you are right. It's too late for such a check in uiomove().
Let's go with your second diff then.

Post by Martin Natano
Unfortunately, I think the best we can do is to audit and fix all
uiomove() callers, which we already do for the purpose of the
conversion.
cheers,
natano
Index: tmpfs_subr.c
===================================================================
RCS file: /cvs/src/sys/tmpfs/tmpfs_subr.c,v
retrieving revision 1.14
diff -u -p -u -r1.14 tmpfs_subr.c
--- tmpfs_subr.c 17 Apr 2015 04:43:21 -0000 1.14
+++ tmpfs_subr.c 17 Jan 2016 12:05:59 -0000
@@ -740,7 +740,7 @@ tmpfs_dir_getdotents(tmpfs_node_t *node,
return EJUSTRETURN;
}
- if ((error = uiomovei(dp, dp->d_reclen, uio)) != 0) {
+ if ((error = uiomove(dp, dp->d_reclen, uio)) != 0) {
return error;
}
@@ -837,7 +837,7 @@ tmpfs_dir_getdents(tmpfs_node_t *node, s
}
/* Copy out the directory entry and continue. */
- error = uiomovei(&dent, dent.d_reclen, uio);
+ error = uiomove(&dent, dent.d_reclen, uio);
if (error) {
break;
}
@@ -1225,7 +1225,7 @@ tmpfs_uiomove(tmpfs_node_t *node, struct
if (pgoff + len < PAGE_SIZE) {
va = tmpfs_uio_lookup(node, pgnum);
if (va != (vaddr_t)NULL)
- return uiomovei((void *)va + pgoff, len, uio);
+ return uiomove((void *)va + pgoff, len, uio);
}
if (len >= TMPFS_UIO_MAXBYTES) {
@@ -1249,7 +1249,7 @@ tmpfs_uiomove(tmpfs_node_t *node, struct
return error;
}
- error = uiomovei((void *)va + pgoff, sz, uio);
+ error = uiomove((void *)va + pgoff, sz, uio);
if (error == 0 && pgoff + sz < PAGE_SIZE)
tmpfs_uio_cache(node, pgnum, va);
else
Index: tmpfs_vnops.c
===================================================================
RCS file: /cvs/src/sys/tmpfs/tmpfs_vnops.c,v
retrieving revision 1.23
diff -u -p -u -r1.23 tmpfs_vnops.c
--- tmpfs_vnops.c 8 Dec 2015 15:26:25 -0000 1.23
+++ tmpfs_vnops.c 17 Jan 2016 12:06:12 -0000
@@ -1017,8 +1017,8 @@ tmpfs_readlink(void *v)
KASSERT(vp->v_type == VLNK);
node = VP_TO_TMPFS_NODE(vp);
- error = uiomovei(node->tn_spec.tn_lnk.tn_link,
- MIN(node->tn_size, uio->uio_resid), uio);
+ error = uiomove(node->tn_spec.tn_lnk.tn_link,
+ MIN((size_t)node->tn_size, uio->uio_resid), uio);
tmpfs_update(node, TMPFS_NODE_ACCESSED);
return error;