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 <[email protected]>
---
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);
- 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;
+ 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
On Tue, Jan 22, 2019 at 09:03:01PM +0100, Mattias Jacobsson wrote:
> In the function wmi_dev_match() there are three variables that
> potentially can result in a null pointer dereference. Namely:
Is this something you have observed? This gets called when a new driver
registered for each unassociated device on the bus, so I'm not
immediately seeing how dev or driver would end up being NULL here.
See: Documentation/driver-model/bus.txt
--
Darren Hart
VMware Open Source Technology Center
Hi Darren,
On 2019-01-26, Darren Hart wrote:
> On Tue, Jan 22, 2019 at 09:03:01PM +0100, Mattias Jacobsson wrote:
> > In the function wmi_dev_match() there are three variables that
> > potentially can result in a null pointer dereference. Namely:
>
> Is this something you have observed? This gets called when a new driver
> registered for each unassociated device on the bus, so I'm not
> immediately seeing how dev or driver would end up being NULL here.
I've observed that wmi_driver->id_table can be NULL, that can happen
when a WMI driver registers itself without specifying an id_table.
When adding a NULL check for wmi_driver->id_table, it felt wrong to not
do the same check for driver. After all, driver is a pointer given to me
from someone else, and can therefore be NULL. The same reasoning applies
to dev.
>
> See: Documentation/driver-model/bus.txt
>
> --
> Darren Hart
> VMware Open Source Technology Center
Thanks,
Mattias
On Tue, Jan 22, 2019 at 10:04 PM Mattias Jacobsson <[email protected]> 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 <[email protected]>
> ---
> 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.
> - 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?
> + 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
Hi,
On 2019-01-27, Andy Shevchenko wrote:
> On Tue, Jan 22, 2019 at 10:04 PM Mattias Jacobsson <[email protected]> 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 <[email protected]>
> > ---
> > 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
On Mon, Jan 28, 2019 at 4:39 PM Mattias Jacobsson <[email protected]> wrote:
> On 2019-01-27, Andy Shevchenko wrote:
> > On Tue, Jan 22, 2019 at 10:04 PM Mattias Jacobsson <[email protected]> wrote:
> > > - 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.
So, I suggest in these cases do not touch existing till the actual
problem will be discovered and proved.
I.o.w. touch only place which you have real problem, and describe this
issue in commit message.
--
With Best Regards,
Andy Shevchenko