On Wed, Feb 21, 2018 at 10:28 PM, Ted Unangst <t...@tedunangst.com> wrote:

> Mateusz Guzik wrote:
> > As the subject states. By the time the code gets to copyout, buf is
> > already populated. Clearing it only if copyout succeeds looks like a
> > braino, thus the following trivial proposal:
>
> If the secret random data is not copied out, it will not be used, and
> there's
> nothing to leak. I think this just complicates the code.
>
>
It is a nit. However, leaving the buf like this gives me an uneasy
feeling and the patch is most definitely not a complication.

That said, your OS - your call.

>
> > diff --git a/sys/dev/rnd.c b/sys/dev/rnd.c
> > index e33cb5fd7c0..fa876a950b9 100644
> > --- a/sys/dev/rnd.c
> > +++ b/sys/dev/rnd.c
> > @@ -932,9 +932,9 @@ sys_getentropy(struct proc *p, void *v, register_t
> > *retval)
> >         if (SCARG(uap, nbyte) > sizeof(buf))
> >                 return (EIO);
> >         arc4random_buf(buf, SCARG(uap, nbyte));
> > -       if ((error = copyout(buf, SCARG(uap, buf), SCARG(uap, nbyte)))
> != 0)
> > -               return (error);
> > +       error = copyout(buf, SCARG(uap, buf), SCARG(uap, nbyte));
> >         explicit_bzero(buf, sizeof(buf));
> > -       retval[0] = 0;
> > -       return (0);
> > +       if (error == 0)
> > +               retval[0] = 0;
> > +       return (error);
> >  }
>

-- 
Mateusz Guzik <mjguzik gmail.com>

Reply via email to