diff mbox series

platform/x86: huawei-wmi: Fix a possible NULL deref

Message ID 20191225235841.14393-1-ayman.bagabas@gmail.com (mailing list archive)
State Rejected, archived
Headers show
Series platform/x86: huawei-wmi: Fix a possible NULL deref | expand

Commit Message

Ayman Bagabas Dec. 25, 2019, 11:58 p.m. UTC 
We're iterating over a NULL terminated array.

Fixes: 1ac9abeb2e5b ("platform/x86: huawei-wmi: Move to platform driver")
Signed-off-by: Ayman Bagabas <ayman.bagabas@gmail.com>
---
 drivers/platform/x86/huawei-wmi.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)


base-commit: 46cf053efec6a3a5f343fead837777efe8252a46

Comments

Ayman Bagabas Dec. 26, 2019, 8:42 p.m. UTC | #1 
Please ignore this patch I'll be sending another one. guid->guid_string
is always true.

On 19/12/25 06:58PM, Ayman Bagabas wrote:
> We're iterating over a NULL terminated array.
>
> Fixes: 1ac9abeb2e5b ("platform/x86: huawei-wmi: Move to platform driver")
> Signed-off-by: Ayman Bagabas <ayman.bagabas@gmail.com>
> ---
>  drivers/platform/x86/huawei-wmi.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/platform/x86/huawei-wmi.c b/drivers/platform/x86/huawei-wmi.c
> index a2d846c4a7ee..42d461eeeff4 100644
> --- a/drivers/platform/x86/huawei-wmi.c
> +++ b/drivers/platform/x86/huawei-wmi.c
> @@ -784,13 +784,13 @@ static const struct wmi_device_id huawei_wmi_events_id_table[] = {
>  static int huawei_wmi_probe(struct platform_device *pdev)
>  {
>  	const struct wmi_device_id *guid = huawei_wmi_events_id_table;
> +	struct input_dev *idev = *huawei_wmi->idev;
>  	int err;
>
>  	platform_set_drvdata(pdev, huawei_wmi);
>  	huawei_wmi->dev = &pdev->dev;
>
> -	while (*guid->guid_string) {
> -		struct input_dev *idev = *huawei_wmi->idev;
> +	while (guid->guid_string) {
>
>  		if (wmi_has_guid(guid->guid_string)) {
>  			err = huawei_wmi_input_setup(&pdev->dev, guid->guid_string, &idev);
> @@ -820,7 +820,7 @@ static int huawei_wmi_remove(struct platform_device *pdev)
>  {
>  	const struct wmi_device_id *guid = huawei_wmi_events_id_table;
>
> -	while (*guid->guid_string) {
> +	while (guid->guid_string) {
>  		if (wmi_has_guid(guid->guid_string))
>  			huawei_wmi_input_exit(&pdev->dev, guid->guid_string);
>
>
> base-commit: 46cf053efec6a3a5f343fead837777efe8252a46
> --
> 2.24.1
>

--
Thank you,
Ayman
Dan Carpenter Dec. 26, 2019, 9:54 p.m. UTC | #2 
On Wed, Dec 25, 2019 at 06:58:38PM -0500, Ayman Bagabas wrote:
> We're iterating over a NULL terminated array.

This changelog is kind of messed up.  This is how it looks in context:
https://marc.info/?l=linux-kernel&m=157731837511760&w=2
The subject and the commit message are far apart.  What's wrong with
iterating over a NULL terminated array?  The changelog doesn't say which
variable is NULL.

> 
> Fixes: 1ac9abeb2e5b ("platform/x86: huawei-wmi: Move to platform driver")
> Signed-off-by: Ayman Bagabas <ayman.bagabas@gmail.com>
> ---
>  drivers/platform/x86/huawei-wmi.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/platform/x86/huawei-wmi.c b/drivers/platform/x86/huawei-wmi.c
> index a2d846c4a7ee..42d461eeeff4 100644
> --- a/drivers/platform/x86/huawei-wmi.c
> +++ b/drivers/platform/x86/huawei-wmi.c
> @@ -784,13 +784,13 @@ static const struct wmi_device_id huawei_wmi_events_id_table[] = {
>  static int huawei_wmi_probe(struct platform_device *pdev)
>  {
>  	const struct wmi_device_id *guid = huawei_wmi_events_id_table;
> +	struct input_dev *idev = *huawei_wmi->idev;

This line seems like an unrelated change.  I'm still not sure the
justification for this.  I really hate puzzling over patches to try
figure out why a patch is making changes.

regards,
dan carpenter
Ayman Bagabas Dec. 27, 2019, 3:57 p.m. UTC | #3 
On 19/12/27 12:54AM, Dan Carpenter wrote:
> On Wed, Dec 25, 2019 at 06:58:38PM -0500, Ayman Bagabas wrote:
> > We're iterating over a NULL terminated array.
>
> This changelog is kind of messed up.  This is how it looks in context:
> https://marc.info/?l=linux-kernel&m=157731837511760&w=2
> The subject and the commit message are far apart.  What's wrong with
> iterating over a NULL terminated array?  The changelog doesn't say which
> variable is NULL.
>

I'm really sorry for my poor subject and commit message that shouldn't happen again.

This is not an issue, the problem occurs to me when I try to use this
module on kernel 5.0, particularly, when iterating over the struct
wmi_device_id array. On kernel 5.0, I'm getting a NULL pointer
dereference on *guid->guid_string on the 3rd NULL struct in the array.
This is happening because the definition of struct wmi_device_id in <5.1 is

struct wmi_device_id {
	const char *guid_string;
};

Compared to this where guid->guid_string is not NULL

struct wmi_device_id {
	const char guid_string[UUID_STRING_LEN+1];
};

> >
> > Fixes: 1ac9abeb2e5b ("platform/x86: huawei-wmi: Move to platform driver")
> > Signed-off-by: Ayman Bagabas <ayman.bagabas@gmail.com>
> > ---
> >  drivers/platform/x86/huawei-wmi.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/platform/x86/huawei-wmi.c b/drivers/platform/x86/huawei-wmi.c
> > index a2d846c4a7ee..42d461eeeff4 100644
> > --- a/drivers/platform/x86/huawei-wmi.c
> > +++ b/drivers/platform/x86/huawei-wmi.c
> > @@ -784,13 +784,13 @@ static const struct wmi_device_id huawei_wmi_events_id_table[] = {
> >  static int huawei_wmi_probe(struct platform_device *pdev)
> >  {
> >  	const struct wmi_device_id *guid = huawei_wmi_events_id_table;
> > +	struct input_dev *idev = *huawei_wmi->idev;
>
> This line seems like an unrelated change.  I'm still not sure the
> justification for this.  I really hate puzzling over patches to try
> figure out why a patch is making changes.

This one is a logical error, we have an array of input_dev pointers for
each guid. Defining idev in the loop would always reset the pointer to
the first element in the array. The address of each pointer then passed
to huawei_wmi_input_setup to allocate an input device. We want to keep a
pointer to each allocated input device in the static huawei_wmi struct.

>
> regards,
> dan carpenter
>
>

--
Thank you,
Ayman
diff mbox series

Patch

diff --git a/drivers/platform/x86/huawei-wmi.c b/drivers/platform/x86/huawei-wmi.c
index a2d846c4a7ee..42d461eeeff4 100644
--- a/drivers/platform/x86/huawei-wmi.c
+++ b/drivers/platform/x86/huawei-wmi.c
@@ -784,13 +784,13 @@  static const struct wmi_device_id huawei_wmi_events_id_table[] = {
 static int huawei_wmi_probe(struct platform_device *pdev)
 {
 	const struct wmi_device_id *guid = huawei_wmi_events_id_table;
+	struct input_dev *idev = *huawei_wmi->idev;
 	int err;
 
 	platform_set_drvdata(pdev, huawei_wmi);
 	huawei_wmi->dev = &pdev->dev;
 
-	while (*guid->guid_string) {
-		struct input_dev *idev = *huawei_wmi->idev;
+	while (guid->guid_string) {
 
 		if (wmi_has_guid(guid->guid_string)) {
 			err = huawei_wmi_input_setup(&pdev->dev, guid->guid_string, &idev);
@@ -820,7 +820,7 @@  static int huawei_wmi_remove(struct platform_device *pdev)
 {
 	const struct wmi_device_id *guid = huawei_wmi_events_id_table;
 
-	while (*guid->guid_string) {
+	while (guid->guid_string) {
 		if (wmi_has_guid(guid->guid_string))
 			huawei_wmi_input_exit(&pdev->dev, guid->guid_string);