Stefan Kempf wrote:
> 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.
> 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;
