Re: [PATCH] staging: vc04_services: Fixed address type mismatch in vchiq_arm.c

[Arnd Bergmann]

On Thu, Feb 18, 2021 at 10:39 AM Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
>
> On Thu, Feb 18, 2021 at 02:40:15PM +0530, Pritthijit Nath wrote:
> > This change fixes a sparse address type mismatch warning "incorrect type
> > in assignment (different address spaces)".
> >
> > Signed-off-by: Pritthijit Nath <pritthijit.nath@xxxxxxxxxx>
> > ---
> >  .../staging/vc04_services/interface/vchiq_arm/vchiq_arm.c   | 6 +-----
> >  1 file changed, 1 insertion(+), 5 deletions(-)
> >
> > 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 59e45dc03a97..3c715b926a57 100644
> > --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> > +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> > @@ -1214,11 +1214,7 @@ static int vchiq_ioc_await_completion(struct vchiq_instance *instance,
> >                   !instance->use_close_delivered)
> >                       unlock_service(service);
> >
> > -             /*
> > -              * FIXME: address space mismatch, does bulk_userdata
> > -              * actually point to user or kernel memory?
> > -              */
> > -             user_completion.bulk_userdata = completion->bulk_userdata;
> > +             user_completion.bulk_userdata = (void __user *)completion->bulk_userdata;
>
> So, this pointer really is user memory?
>
> How did you determine that?
>
> If so, why isn't this a __user * in the first place?
>
> You can't just paper over the FIXME by doing a cast without doing the
> real work here, otherwise someone wouldn't have written the FIXME :)

Agreed. I added the FIXME as part of a cleanup work I did last year.
The obvious step is to mark the corresponding field in
vchiq_completion_data_kernel as a __user pointer, and then check
all assignments *to* that members to ensure they all refer to __user
pointers as well.

At some point I gave up here, as far as I recall there were certain
assignments that were clearly kernel data, in particular the
vchiq_service_params_kernel->callback() argument seems to
sometimes come from kmalloc() and must not be passed down
to user space.

The alternative would be to look at the user space side to figure
out how the returned data is actually used. If user space doesn't
rely on it, it can simply get set to NULL, and if it does use it,
then the question is which code path in the kernel correctly
assigns it.

        Arnd