* [PATCH] platform/x86: wmi: fix potential null pointer dereferences
@ 2019-01-22 20:03 Mattias Jacobsson
2019-01-26 20:43 ` Darren Hart
2019-01-27 20:32 ` Andy Shevchenko
0 siblings, 2 replies; 6+ messages in thread
From: Mattias Jacobsson @ 2019-01-22 20:03 UTC (permalink / raw)
To: dvhart, andy; +Cc: 2pi, platform-driver-x86, linux-kernel
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);
- 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
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] platform/x86: wmi: fix potential null pointer dereferences
2019-01-22 20:03 [PATCH] platform/x86: wmi: fix potential null pointer dereferences Mattias Jacobsson
@ 2019-01-26 20:43 ` Darren Hart
2019-01-27 11:46 ` Mattias Jacobsson
2019-01-27 20:32 ` Andy Shevchenko
1 sibling, 1 reply; 6+ messages in thread
From: Darren Hart @ 2019-01-26 20:43 UTC (permalink / raw)
To: Mattias Jacobsson; +Cc: andy, platform-driver-x86, linux-kernel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] platform/x86: wmi: fix potential null pointer dereferences
2019-01-26 20:43 ` Darren Hart
@ 2019-01-27 11:46 ` Mattias Jacobsson
0 siblings, 0 replies; 6+ messages in thread
From: Mattias Jacobsson @ 2019-01-27 11:46 UTC (permalink / raw)
To: Darren Hart; +Cc: andy, platform-driver-x86, linux-kernel, 2pi
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] platform/x86: wmi: fix potential null pointer dereferences
2019-01-22 20:03 [PATCH] platform/x86: wmi: fix potential null pointer dereferences Mattias Jacobsson
2019-01-26 20:43 ` Darren Hart
@ 2019-01-27 20:32 ` Andy Shevchenko
2019-01-28 14:38 ` Mattias Jacobsson
1 sibling, 1 reply; 6+ messages in thread
From: Andy Shevchenko @ 2019-01-27 20:32 UTC (permalink / raw)
To: Mattias Jacobsson
Cc: Darren Hart, Andy Shevchenko, Platform Driver, Linux Kernel Mailing List
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.
> - 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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] platform/x86: wmi: fix potential null pointer dereferences
2019-01-27 20:32 ` Andy Shevchenko
@ 2019-01-28 14:38 ` Mattias Jacobsson
2019-01-28 15:49 ` Andy Shevchenko
0 siblings, 1 reply; 6+ messages in thread
From: Mattias Jacobsson @ 2019-01-28 14:38 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Darren Hart, Andy Shevchenko, Platform Driver,
Linux Kernel Mailing List, 2pi
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] platform/x86: wmi: fix potential null pointer dereferences
2019-01-28 14:38 ` Mattias Jacobsson
@ 2019-01-28 15:49 ` Andy Shevchenko
0 siblings, 0 replies; 6+ messages in thread
From: Andy Shevchenko @ 2019-01-28 15:49 UTC (permalink / raw)
To: Mattias Jacobsson
Cc: Darren Hart, Andy Shevchenko, Platform Driver, Linux Kernel Mailing List
On Mon, Jan 28, 2019 at 4:39 PM Mattias Jacobsson <2pi@mok.nu> wrote:
> On 2019-01-27, Andy Shevchenko wrote:
> > On Tue, Jan 22, 2019 at 10:04 PM Mattias Jacobsson <2pi@mok.nu> 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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-01-28 15:49 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-01-22 20:03 [PATCH] platform/x86: wmi: fix potential null pointer dereferences Mattias Jacobsson
2019-01-26 20:43 ` Darren Hart
2019-01-27 11:46 ` Mattias Jacobsson
2019-01-27 20:32 ` Andy Shevchenko
2019-01-28 14:38 ` Mattias Jacobsson
2019-01-28 15:49 ` Andy Shevchenko
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).