[PATCH 1/2] staging: vchiq: Fix bulk userdata handling

[Dan Carpenter]

On Tue, Jan 05, 2021 at 11:53:32AM +0000, Phil Elwell wrote:
> On Tue, 5 Jan 2021 at 11:04, Dan Carpenter <dan.carpenter at oracle.com> wrote:
> >
> > On Mon, Jan 04, 2021 at 07:26:42PM +0000, Phil Elwell wrote:
> > > On 04/01/2021 18:31, Dan Carpenter wrote:
> > > > On Mon, Jan 04, 2021 at 12:09:27PM +0000, Phil Elwell wrote:
> > > > > The addition of the local 'userdata' pointer to
> > > > > vchiq_irq_queue_bulk_tx_rx omitted the case where neither BLOCKING nor
> > > > > WAITING modes are used, in which case the value provided by the
> > > > > caller is replaced with a NULL.
> > > > >
> > > > > Fixes: 4184da4f316a ("staging: vchiq: fix __user annotations")
> > > > >
> > > > > Signed-off-by: Phil Elwell <phil at raspberrypi.com>
> > > > > ---
> > > > >   drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c | 4 +++-
> > > > >   1 file changed, 3 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> > > > > index f500a7043805..2a8883673ba1 100644
> > > > > --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> > > > > +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> > > > > @@ -958,7 +958,7 @@ static int vchiq_irq_queue_bulk_tx_rx(struct vchiq_instance *instance,
> > > > >           struct vchiq_service *service;
> > > > >           struct bulk_waiter_node *waiter = NULL;
> > > > >           bool found = false;
> > > > > - void *userdata = NULL;
> > > > > + void *userdata;
> > > > >           int status = 0;
> > > > >           int ret;
> > > > > @@ -997,6 +997,8 @@ static int vchiq_irq_queue_bulk_tx_rx(struct vchiq_instance *instance,
> > > > >                           "found bulk_waiter %pK for pid %d", waiter,
> > > > >                           current->pid);
> > > > >                   userdata = &waiter->bulk_waiter;
> > > > > + } else {
> > > > > +         userdata = args->userdata;
> > > >
> > > > "args->userdata" is marked as a user pointer so we really don't want to
> > > > mix user and kernel pointers here.  Presumably this opens up a large
> > > > security hole.
> > >
> > > It's an opaque, pointer-sized token that only exists to bereturned to userspace (or not,
> > > without this patch) - it's hard to see that as a security hole.
> >
> > I was assuming the bug here was a NULL dereference...  Apparently that's
> > not the case?  The commit message needs to be updated to be more clear
> > about how the bug looks like to the user.
> >
> > Are we using the "&waiter->bulk_waiter" as a "token to be returned to
> > userspace" as well?  It looks like maybe it is in vchiq_put_completion().
> > That defeats KASLR and is a different sort of security problem.
> >
> > Mixing __user pointers and regular pointers is dangerous and has lead to
> > security problems in this driver in the past.  But also mixing mixing
> > tokens with pointers just makes the code hard to read.  Instead of
> > undoing Arnd's work where he split the user space and kernel pointers
> > apart we should go ahead and spit it up even more.  At least add a giant
> > FIXME comment and an item in the TODO list so we don't forget to do this
> > before removing the code from staging.
> 
> Those all sound like valid comments to have made against the original
> patch, but that seems to have received little attention.
> 
> I'll just leave this here - perhaps Arnd has the patience to finish the job.

I kind of have a headache today so maybe I shouldn't be sending emails.
But really, all I'm asking is for is two fairly reasonable things:

1) The commit message needs to say what the bug looks like to the user.
   Up to now, I still have no idea the answer to this question.

2) Put a note in the TODO which says: "Clean up Sparse warnings from
   __user annotations.  See vchiq_irq_queue_bulk_tx_rx().  Ensure that
   the the address of "&waiter->bulk_waiter" is never disclosed to
   userspace."

regards,
dan carpenter