On Wed, 14 Feb 2024, Armin Wolf wrote:
> WMI event drivers which do not have no_notify_data set expect
> that each WMI event contains valid data. Evaluating _WED however
> might return no data, which can cause issues with such drivers.
>
> Fix this by validating that evaluating _WED did return data.
>
> Signed-off-by: Armin Wolf <[email protected]>
> ---
> drivers/platform/x86/wmi.c | 11 +++++++++--
> 1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
> index 34d8f55afaad..8a916887c546 100644
> --- a/drivers/platform/x86/wmi.c
> +++ b/drivers/platform/x86/wmi.c
> @@ -1211,6 +1211,7 @@ static void wmi_notify_driver(struct wmi_block *wblock)
> {
> struct wmi_driver *driver = drv_to_wdrv(wblock->dev.dev.driver);
> struct acpi_buffer data = { ACPI_ALLOCATE_BUFFER, NULL };
> + union acpi_object *obj = NULL;
> acpi_status status;
>
> if (!driver->no_notify_data) {
> @@ -1219,12 +1220,18 @@ static void wmi_notify_driver(struct wmi_block *wblock)
> dev_warn(&wblock->dev.dev, "Failed to get event data\n");
> return;
> }
> +
> + obj = data.pointer;
> + if (!obj) {
> + dev_warn(&wblock->dev.dev, "Event contains not event data\n");
> + return;
> + }
> }
>
> if (driver->notify)
> - driver->notify(&wblock->dev, data.pointer);
> + driver->notify(&wblock->dev, obj);
>
> - kfree(data.pointer);
> + kfree(obj);
Hi Armin,
While looking into this patch, I failed to connect the mention of
no_notify_data in the commit message with the code change that does
nothing differently based no_notify_data being set or not, AFAICT.
It could be just that you need to explain things better in the commit
message, I'm not sure.
--
i.
Am 15.02.24 um 13:31 schrieb Ilpo Järvinen:
> On Wed, 14 Feb 2024, Armin Wolf wrote:
>
>> WMI event drivers which do not have no_notify_data set expect
>> that each WMI event contains valid data. Evaluating _WED however
>> might return no data, which can cause issues with such drivers.
>>
>> Fix this by validating that evaluating _WED did return data.
>>
>> Signed-off-by: Armin Wolf <[email protected]>
>> ---
>> drivers/platform/x86/wmi.c | 11 +++++++++--
>> 1 file changed, 9 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
>> index 34d8f55afaad..8a916887c546 100644
>> --- a/drivers/platform/x86/wmi.c
>> +++ b/drivers/platform/x86/wmi.c
>> @@ -1211,6 +1211,7 @@ static void wmi_notify_driver(struct wmi_block *wblock)
>> {
>> struct wmi_driver *driver = drv_to_wdrv(wblock->dev.dev.driver);
>> struct acpi_buffer data = { ACPI_ALLOCATE_BUFFER, NULL };
>> + union acpi_object *obj = NULL;
>> acpi_status status;
>>
>> if (!driver->no_notify_data) {
>> @@ -1219,12 +1220,18 @@ static void wmi_notify_driver(struct wmi_block *wblock)
>> dev_warn(&wblock->dev.dev, "Failed to get event data\n");
>> return;
>> }
>> +
>> + obj = data.pointer;
>> + if (!obj) {
>> + dev_warn(&wblock->dev.dev, "Event contains not event data\n");
>> + return;
>> + }
>> }
>>
>> if (driver->notify)
>> - driver->notify(&wblock->dev, data.pointer);
>> + driver->notify(&wblock->dev, obj);
>>
>> - kfree(data.pointer);
>> + kfree(obj);
> Hi Armin,
>
> While looking into this patch, I failed to connect the mention of
> no_notify_data in the commit message with the code change that does
> nothing differently based no_notify_data being set or not, AFAICT.
>
> It could be just that you need to explain things better in the commit
> message, I'm not sure.
Here the _WED ACPI control method is only evaluated if driver->no_notify_data is not set.
So the returned ACPI object should only be validated in this case, as we pass NULL otherwise.
Armin Wolf
On Thu, 15 Feb 2024, Armin Wolf wrote:
> Am 15.02.24 um 13:31 schrieb Ilpo Järvinen:
>
> > On Wed, 14 Feb 2024, Armin Wolf wrote:
> >
> > > WMI event drivers which do not have no_notify_data set expect
> > > that each WMI event contains valid data. Evaluating _WED however
> > > might return no data, which can cause issues with such drivers.
> > >
> > > Fix this by validating that evaluating _WED did return data.
> > >
> > > Signed-off-by: Armin Wolf <[email protected]>
> > > ---
> > > drivers/platform/x86/wmi.c | 11 +++++++++--
> > > 1 file changed, 9 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
> > > index 34d8f55afaad..8a916887c546 100644
> > > --- a/drivers/platform/x86/wmi.c
> > > +++ b/drivers/platform/x86/wmi.c
> > > @@ -1211,6 +1211,7 @@ static void wmi_notify_driver(struct wmi_block
> > > *wblock)
> > > {
> > > struct wmi_driver *driver = drv_to_wdrv(wblock->dev.dev.driver);
> > > struct acpi_buffer data = { ACPI_ALLOCATE_BUFFER, NULL };
> > > + union acpi_object *obj = NULL;
> > > acpi_status status;
> > >
> > > if (!driver->no_notify_data) {
> > > @@ -1219,12 +1220,18 @@ static void wmi_notify_driver(struct wmi_block
> > > *wblock)
> > > dev_warn(&wblock->dev.dev, "Failed to get event
> > > data\n");
> > > return;
> > > }
> > > +
> > > + obj = data.pointer;
> > > + if (!obj) {
> > > + dev_warn(&wblock->dev.dev, "Event contains not event
> > > data\n");
> > > + return;
> > > + }
> > > }
> > >
> > > if (driver->notify)
> > > - driver->notify(&wblock->dev, data.pointer);
> > > + driver->notify(&wblock->dev, obj);
> > >
> > > - kfree(data.pointer);
> > > + kfree(obj);
> > Hi Armin,
> >
> > While looking into this patch, I failed to connect the mention of
> > no_notify_data in the commit message with the code change that does
> > nothing differently based no_notify_data being set or not, AFAICT.
> >
> > It could be just that you need to explain things better in the commit
> > message, I'm not sure.
>
> Here the _WED ACPI control method is only evaluated if driver->no_notify_data
> is not set.
> So the returned ACPI object should only be validated in this case, as we pass
> NULL otherwise.
Yes, I'm sorry, it seems fine. For some reason I was very confused while
reviewing even if no_notify_data was mentioned right in the previous
context (maybe Iused some older version of the code while trying to figure
things out, I dunno).
--
i.