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@xxxxxxxxxx> 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@xxxxxxxxxxxxxxx>
> > > > > ---
> > > > > 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
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
