linux-kernel - Re: [PATCH] staging: vc04_services: Fixed address type mismatch in vchiq

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAK8P3a09TXVwZYz+ohU1uJh5XvEJ=aHqUq+hs2K-4nvppbTPSA@mail.gmail.com>
Date:   Thu, 18 Feb 2021 18:10:48 +0100
From:   Arnd Bergmann <arnd@...nel.org>
To:     Greg KH <gregkh@...uxfoundation.org>
Cc:     Pritthijit Nath <pritthijit.nath@...oud.com>,
        Nicolas Saenz Julienne <nsaenzjulienne@...e.de>,
        Arnd Bergmann <arnd@...db.de>,
        Dan Carpenter <dan.carpenter@...cle.com>,
        Amarjargal Gundjalam <amarjargal16@...il.com>,
        Phil Elwell <phil@...pberrypi.com>,
        driverdevel <devel@...verdev.osuosl.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        bcm-kernel-feedback-list <bcm-kernel-feedback-list@...adcom.com>,
        "moderated list:BROADCOM BCM2835 ARM ARCHITECTURE" 
        <linux-rpi-kernel@...ts.infradead.org>,
        Linux ARM <linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH] staging: vc04_services: Fixed address type mismatch in vchiq_arm.c

On Thu, Feb 18, 2021 at 10:39 AM Greg KH <gregkh@...uxfoundation.org> 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@...oud.com>
> > ---
> >  .../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