Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Mon, 28 Jan 2019 15:38:45 +0100
From:   Mattias Jacobsson <2pi@mok.nu>
To:     Andy Shevchenko <andy.shevchenko@gmail.com>
CC:     Darren Hart <dvhart@infradead.org>,
        Andy Shevchenko <andy@infradead.org>,
        Platform Driver <platform-driver-x86@vger.kernel.org>,
        "Linux Kernel Mailing List" <linux-kernel@vger.kernel.org>,
        <2pi@mok.nu>
Subject: Re: [PATCH] platform/x86: wmi: fix potential null pointer
 dereferences
Message-ID: <20190128143845.2gneic5ip5b56mwv@mok.nu>
References: <20190122200302.19861-1-2pi@mok.nu>
 <CAHp75Vfv1sMyAQn=ue0hroqAnbkhQayCj64xNBHO5PRRv53GSA@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <CAHp75Vfv1sMyAQn=ue0hroqAnbkhQayCj64xNBHO5PRRv53GSA@mail.gmail.com>
Received-SPF: Neutral (EDGE01LEVEL1.hmc.local: 192.168.46.11 is neither
 permitted nor denied by domain of 2pi@mok.nu)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi,

On 2019-01-27, Andy Shevchenko wrote:
> On Tue, Jan 22, 2019 at 10:04 PM Mattias Jacobsson <2pi@mok.nu> wrote:
> >
> > In the function wmi_dev_match() there are three variables that
> > potentially can result in a null pointer dereference. Namely:
> > dev/wblock, driver/wmi_driver, and wmi_driver->id_table.
> >
> > Check for NULL and return that the driver can't handle the device if any
> > of these variables would result in a null pointer dereference.
> >
> > The NULL checks are performed prior to running container_of() for the
> > variables dev/wblock and driver/wmi_driver.
> >
> > Fixes: 844af950da94 ("platform/x86: wmi: Turn WMI into a bus driver")
> > Signed-off-by: Mattias Jacobsson <2pi@mok.nu>
> > ---
> >  drivers/platform/x86/wmi.c | 16 ++++++++++++----
> >  1 file changed, 12 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
> > index bea35be68706..c596479e8b13 100644
> > --- a/drivers/platform/x86/wmi.c
> > +++ b/drivers/platform/x86/wmi.c
> > @@ -763,10 +763,18 @@ static void wmi_dev_release(struct device *dev)
> >
> >  static int wmi_dev_match(struct device *dev, struct device_driver *driver)
> >  {
> > -       struct wmi_driver *wmi_driver =
> > -               container_of(driver, struct wmi_driver, driver);
> 
> AFAIU this is just a pointer arithmetics, no need to move it.

That is my understanding too, it seamed backwards to do the NULL check
afterwards, but we still have access to dev and driver. So why not...

> 
> > -       struct wmi_block *wblock = dev_to_wblock(dev);
> 
> > -       const struct wmi_device_id *id = wmi_driver->id_table;
> > +       const struct wmi_device_id *id;
> > +       struct wmi_block *wblock;
> > +       struct wmi_driver *wmi_driver;
> > +
> 
> > +       if (dev == NULL || driver == NULL)
> > +               return 0;
> 
> On which circumstances this may ever happen?

Nothing in particular. If there is a bug in the caller of this function,
then that is when this will come into play. See my earlier mail to
Darren too.

> 
> > +       wblock = dev_to_wblock(dev);
> > +       wmi_driver = container_of(driver, struct wmi_driver, driver);
> > +
> > +       if (wmi_driver->id_table == NULL)
> > +               return 0;
> > +       id = wmi_driver->id_table;
> >
> >         while (id->guid_string) {
> >                 uuid_le driver_guid;
> > --
> > 2.20.1
> >
> 
> 
> -- 
> With Best Regards,
> Andy Shevchenko

Thanks,
Mattias