Martin Natano wrote:
> 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.
D'oh. Yes, you are right. It's too late for such a check in uiomove().
Let's go with your second diff then.
> 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;
