[PATCH] Input: xpad - fix GPF in xpad_probe

[Dongliang Mu]

In xpad_probe(), it does not allocate xpad->dev with input_dev type.
Then, when it invokes dev_warn with 1st argument - &xpad->dev->dev, it
would trigger GPF.

Fix this by allocating xpad->dev, its error handling and cleanup
operations in the remove function.

Note that this crash does not have any reproducer, so the patch
only passes compilation testing.

Reported-by: syzbot+a3f758...@syzkaller.appspotmail.com
Signed-off-by: Dongliang Mu <dz...@hust.edu.cn>
---
drivers/input/joystick/xpad.c | 11 +++++++++--
1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/input/joystick/xpad.c b/drivers/input/joystick/xpad.c
index 66a92691a047..2e077b52f46a 100644
--- a/drivers/input/joystick/xpad.c
+++ b/drivers/input/joystick/xpad.c
@@ -1944,6 +1944,7 @@ static int xpad_probe(struct usb_interface *intf, const struct usb_device_id *id
{
struct usb_device *udev = interface_to_usbdev(intf);
struct usb_xpad *xpad;
+ struct input_dev *input_dev;
struct usb_endpoint_descriptor *ep_irq_in, *ep_irq_out;
int i, error;

@@ -1957,9 +1958,13 @@ static int xpad_probe(struct usb_interface *intf, const struct usb_device_id *id
}

xpad = kzalloc(sizeof(struct usb_xpad), GFP_KERNEL);
- if (!xpad)
- return -ENOMEM;
+ input_dev = input_allocate_device();
+ if (!xpad || !input_dev) {
+ error = -ENOMEM;
+ goto err_free_mem;
+ }

+ xpad->dev = input_dev;
usb_make_path(udev, xpad->phys, sizeof(xpad->phys));
strlcat(xpad->phys, "/input0", sizeof(xpad->phys));

@@ -2134,6 +2139,7 @@ static int xpad_probe(struct usb_interface *intf, const struct usb_device_id *id
err_free_idata:
usb_free_coherent(udev, XPAD_PKT_LEN, xpad->idata, xpad->idata_dma);
err_free_mem:
+ input_free_device(input_dev);
kfree(xpad);
return error;
}
@@ -2159,6 +2165,7 @@ static void xpad_disconnect(struct usb_interface *intf)
usb_free_coherent(xpad->udev, XPAD_PKT_LEN,
xpad->idata, xpad->idata_dma);

+ input_free_device(xpad->dev);
kfree(xpad);

usb_set_intfdata(intf, NULL);
--
2.39.2

[Dongliang Mu]

On 2023/4/14 20:55, Dongliang Mu wrote:
> In xpad_probe(), it does not allocate xpad->dev with input_dev type.
> Then, when it invokes dev_warn with 1st argument - &xpad->dev->dev, it
> would trigger GPF.
>
> Fix this by allocating xpad->dev, its error handling and cleanup
> operations in the remove function.
>
> Note that this crash does not have any reproducer, so the patch
> only passes compilation testing.
>
> Reported-by: syzbot+a3f758...@syzkaller.appspotmail.com
> Signed-off-by: Dongliang Mu <dz...@hust.edu.cn>

Hi Dan,

Can you please help take a look at this patch? Since Syzkaller does not
have any reproducer, I cannot test it with reproducers,

therefore, I write this patch all from manual code review.

Dongliang Mu

[Dan Carpenter]

On Fri, Apr 14, 2023 at 08:55:47PM +0800, Dongliang Mu wrote:
> In xpad_probe(), it does not allocate xpad->dev with input_dev type.
> Then, when it invokes dev_warn with 1st argument - &xpad->dev->dev, it
> would trigger GPF.

What is a call tree for this? Actually I found it from the bug report.
drivers/input/joystick/xpad.c
2034 if (error)
2035 dev_warn(&xpad->dev->dev,
2036 "unable to receive magic message: %d\n",
2037 error);
2038 }

>
> Fix this by allocating xpad->dev, its error handling and cleanup
> operations in the remove function.
>
> Note that this crash does not have any reproducer, so the patch
> only passes compilation testing.

The xpad->dev = input_dev; already happens in xpad_init_input(). We
shouldn't allocate it twice. I think the fix is to just use a different
device pointer for the dev_warn(). Why not use &xpad->intf->dev?

>
> Reported-by: syzbot+a3f758...@syzkaller.appspotmail.com

Could you use a Link tag to link to the bug report?
Link: https://groups.google.com/g/syzkaller-bugs/c/iMhTgpGuIbM

This needs a Fixes tag.

Fixes: db7220c48d8d ("Input: xpad - fix support for some third-party controllers")

regards,
dan carpenter

[Dongliang Mu]

On 2023/4/17 17:25, Dan Carpenter wrote:
> On Fri, Apr 14, 2023 at 08:55:47PM +0800, Dongliang Mu wrote:
>> In xpad_probe(), it does not allocate xpad->dev with input_dev type.
>> Then, when it invokes dev_warn with 1st argument - &xpad->dev->dev, it
>> would trigger GPF.
> What is a call tree for this? Actually I found it from the bug report.
> drivers/input/joystick/xpad.c
> 2034 if (error)
> 2035 dev_warn(&xpad->dev->dev,
> 2036 "unable to receive magic message: %d\n",
> 2037 error);
> 2038 }

Hi Dan,

this only occurs in linux-next tree.

https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/input/joystick/xpad.c?n2053#n2053

>> Fix this by allocating xpad->dev, its error handling and cleanup
>> operations in the remove function.
>>
>> Note that this crash does not have any reproducer, so the patch
>> only passes compilation testing.
> The xpad->dev = input_dev; already happens in xpad_init_input(). We
> shouldn't allocate it twice. I think the fix is to just use a different
> device pointer for the dev_warn(). Why not use &xpad->intf->dev?

Yeah, the allocation and assignment is in the last part that I missed
before. We have two choices to fix this issue:

1. Change to another device pointer;

2. Move the allocation and assignment to a previous code site;

If there is no other places dereferencing this pointer before the
allocation and assignment, it's better to use the 1st one.

Let me craft a v2 patch.

>
>> Reported-by: syzbot+a3f758...@syzkaller.appspotmail.com
> Could you use a Link tag to link to the bug report?
> Link: https://groups.google.com/g/syzkaller-bugs/c/iMhTgpGuIbM

Sure, no problem.

[Vicki Pfau]

Hi,

On 4/17/23 03:01, Dongliang Mu wrote:
>
> On 2023/4/17 17:25, Dan Carpenter wrote:
>> On Fri, Apr 14, 2023 at 08:55:47PM +0800, Dongliang Mu wrote:
>>> In xpad_probe(), it does not allocate xpad->dev with input_dev type.
>>> Then, when it invokes dev_warn with 1st argument - &xpad->dev->dev, it
>>> would trigger GPF.
>> What is a call tree for this?  Actually I found it from the bug report.
>> drivers/input/joystick/xpad.c
>>    2034                  if (error)
>>    2035                          dev_warn(&xpad->dev->dev,
>>    2036                                   "unable to receive magic message: %d\n",
>>    2037                                   error);
>>    2038          }
>

Sorry, this appears to be my code, and was merged recently after a few drafts with Dmitry. This code is sensitive to being moved and only affects some controllers, so I'm looking into if I can move it into after creation of the input_dev right now. It's something I'd already thought might be necessary, but I didn't find any evidence for it before. I'll try to get back to you on that soon.

> Hi Dan,
>
> this only occurs in linux-next tree.
>
> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/input/joystick/xpad.c?n2053#n2053
>
>>> Fix this by allocating xpad->dev, its error handling and cleanup
>>> operations in the remove function.
>>>
>>> Note that this crash does not have any reproducer, so the patch
>>> only passes compilation testing.
>> The xpad->dev = input_dev; already happens in xpad_init_input().  We
>> shouldn't allocate it twice.  I think the fix is to just use a different
>> device pointer for the dev_warn().  Why not use &xpad->intf->dev?
>
> Yeah, the allocation and assignment is in the last part that I missed before. We have two choices to fix this issue:
>
> 1. Change to another device pointer;
>
> 2. Move the allocation and assignment to a previous code site;
>
> If there is no other places dereferencing this pointer before the allocation and assignment, it's better to use the 1st one.
>
> Let me craft a v2 patch.
>
>>
>>> Reported-by: syzbot+a3f758...@syzkaller.appspotmail.com
>> Could you use a Link tag to link to the bug report?
>> Link: https://groups.google.com/g/syzkaller-bugs/c/iMhTgpGuIbM
> Sure, no problem.
>>
>> This needs a Fixes tag.
>>
>> Fixes: db7220c48d8d ("Input: xpad - fix support for some third-party controllers")
>>
>> regards,
>> dan carpenter
>>

Vicki

[Dongliang Mu]

On 2023/4/17 18:24, Vicki Pfau wrote:
> Hi,
>
> On 4/17/23 03:01, Dongliang Mu wrote:
>> On 2023/4/17 17:25, Dan Carpenter wrote:
>>> On Fri, Apr 14, 2023 at 08:55:47PM +0800, Dongliang Mu wrote:
>>>> In xpad_probe(), it does not allocate xpad->dev with input_dev type.
>>>> Then, when it invokes dev_warn with 1st argument - &xpad->dev->dev, it
>>>> would trigger GPF.
>>> What is a call tree for this?  Actually I found it from the bug report.
>>> drivers/input/joystick/xpad.c
>>>    2034                  if (error)
>>>    2035                          dev_warn(&xpad->dev->dev,
>>>    2036                                   "unable to receive magic message: %d\n",
>>>    2037                                   error);
>>>    2038          }
> Sorry, this appears to be my code, and was merged recently after a few drafts with Dmitry. This code is sensitive to being moved and only affects some controllers, so I'm looking into if I can move it into after creation of the input_dev right now. It's something I'd already thought might be necessary, but I didn't find any evidence for it before. I'll try to get back to you on that soon.

If this is necessary, we can change it with another device pointer.
Otherwise, we need to move it after the allocation and assignment. Or
move the allocation and assignment before which is not suggested.

Thanks for your reply. Do I need to submit a v2 patch? Or you will take
care of it?

Dongliang Mu

[Dan Carpenter]

Btw, we should be thinking about how to detect these sorts of issues
using static analysis. Unfortunately, it's not as simple as saying
"We know this variable is NULL so don't dereference it." The problem
with that is that many times Smatch sees where a pointer is set to NULL
but not when it is assigned to a different value.

What we could do instead is say:
1) If a pointer is dereferenced and we know it is NULL then:
set_state_expr(my_id, expr, &suspicious);
2) If we set a pointer to non-NULL and it is marked as suspicious then
print a warning.

This would generate a warning for cases where we dereference a pointer
before it has been initialized.

It is not hard to write a Smatch check like this. The first draft
approach is only three functions long.

regards,
dan carpenter

[Vicki Pfau]

On 4/17/23 03:33, Dongliang Mu wrote:
>
> On 2023/4/17 18:24, Vicki Pfau wrote:
>> Hi,
>>
>> On 4/17/23 03:01, Dongliang Mu wrote:
>>> On 2023/4/17 17:25, Dan Carpenter wrote:
>>>> On Fri, Apr 14, 2023 at 08:55:47PM +0800, Dongliang Mu wrote:
>>>>> In xpad_probe(), it does not allocate xpad->dev with input_dev type.
>>>>> Then, when it invokes dev_warn with 1st argument - &xpad->dev->dev, it
>>>>> would trigger GPF.
>>>> What is a call tree for this?  Actually I found it from the bug report.
>>>> drivers/input/joystick/xpad.c
>>>>     2034                  if (error)
>>>>     2035                          dev_warn(&xpad->dev->dev,
>>>>     2036                                   "unable to receive magic message: %d\n",
>>>>     2037                                   error);
>>>>     2038          }
>> Sorry, this appears to be my code, and was merged recently after a few drafts with Dmitry. This code is sensitive to being moved and only affects some controllers, so I'm looking into if I can move it into after creation of the input_dev right now. It's something I'd already thought might be necessary, but I didn't find any evidence for it before. I'll try to get back to you on that soon.
>
> If this is necessary, we can change it with another device pointer. Otherwise, we need to move it after the allocation and assignment. Or move the allocation and assignment before which is not suggested.
>
> Thanks for your reply. Do I need to submit a v2 patch? Or you will take care of it?

I'll take care of it. I have a patch prepared, but I need to do a bit more testing before I can confirm it doesn't break one specific controller. I'll try to file it as soon as possible. Do you have a timeframe you need this by?

Vicki

[Dongliang Mu]

On 2023/4/17 19:07, Vicki Pfau wrote:
>
> On 4/17/23 03:33, Dongliang Mu wrote:
>> On 2023/4/17 18:24, Vicki Pfau wrote:
>>> Hi,
>>>
>>> On 4/17/23 03:01, Dongliang Mu wrote:
>>>> On 2023/4/17 17:25, Dan Carpenter wrote:
>>>>> On Fri, Apr 14, 2023 at 08:55:47PM +0800, Dongliang Mu wrote:
>>>>>> In xpad_probe(), it does not allocate xpad->dev with input_dev type.
>>>>>> Then, when it invokes dev_warn with 1st argument - &xpad->dev->dev, it
>>>>>> would trigger GPF.
>>>>> What is a call tree for this?  Actually I found it from the bug report.
>>>>> drivers/input/joystick/xpad.c
>>>>>     2034                  if (error)
>>>>>     2035                          dev_warn(&xpad->dev->dev,
>>>>>     2036                                   "unable to receive magic message: %d\n",
>>>>>     2037                                   error);
>>>>>     2038          }
>>> Sorry, this appears to be my code, and was merged recently after a few drafts with Dmitry. This code is sensitive to being moved and only affects some controllers, so I'm looking into if I can move it into after creation of the input_dev right now. It's something I'd already thought might be necessary, but I didn't find any evidence for it before. I'll try to get back to you on that soon.
>> If this is necessary, we can change it with another device pointer. Otherwise, we need to move it after the allocation and assignment. Or move the allocation and assignment before which is not suggested.
>>
>> Thanks for your reply. Do I need to submit a v2 patch? Or you will take care of it?
> I'll take care of it. I have a patch prepared, but I need to do a bit more testing before I can confirm it doesn't break one specific controller. I'll try to file it as soon as possible. Do you have a timeframe you need this by?

You can follow your own plan since I am an enthusiast who discovers and
patches security vulnerabilities, instead of reply on the functionability.

[Dan Carpenter]

On Mon, Apr 17, 2023 at 01:42:21PM +0300, Dan Carpenter wrote:
> Btw, we should be thinking about how to detect these sorts of issues
> using static analysis. Unfortunately, it's not as simple as saying
> "We know this variable is NULL so don't dereference it." The problem
> with that is that many times Smatch sees where a pointer is set to NULL
> but not when it is assigned to a different value.
>
> What we could do instead is say:
> 1) If a pointer is dereferenced and we know it is NULL then:
> set_state_expr(my_id, expr, &suspicious);
> 2) If we set a pointer to non-NULL and it is marked as suspicious then
> print a warning.

I was thinking about this and it's not so simple. Normally after a
warning we return so the state never transitions from &suspicious to
non-NULL.

So what we could do is set the state to &suspicious. Then at the end of
the function we look at all the states at the return paths. If the
state is non-NULL on any return path then print a warning. This is easy
enough to do, but requires quite a bit of Smatch knowledge so I have
done it. Attached.

Unfortunately, it doesn't print a warning in this case because Smatch
doesn't track that _dev_warn() dereferences the "dev" pointer. The
__dev_printk() function only dereferences "dev" if it is non-NULL.
Smatch only counts it if it *always* dereferences it.

This could be fixed in two steps:
Step 1: track dereferences based on the return insead just yes/no.
Step 2: split _dev_warn() returns into two returns based on if dev is
NULL or non-NULL.

Step 1 is probably a good idea. Step 2 is a bad idea, because it makes
no sense to pass a NULL to dev_warn().

A better approach for this bug is to print a warning if people pass
the address of the offset from a NULL pointer. Combine that with the
same return states filter as earlier to eliminate false positives where
Smatch thinks a pointer is always NULL.

drivers/input/joystick/xpad.c:2053 xpad_probe() warn: address of NULL pointer 'xpad->dev'
drivers/media/i2c/ccs/ccs-data.c:524 ccs_data_parse_rules() warn: address of NULL pointer 'rules'
drivers/scsi/lpfc/lpfc_attr.c:1482 lpfc_reset_pci_bus() warn: address of NULL pointer 'phba->pcidev'

That check is attached too.

regards,
dan carpenter

[Dan Carpenter]

The warnings from this are quite promising.

When you're writing a check, you start with a simple idea and then try
it and then filter out the common false positives.

The first 10 warnings are from loops like:

p = NULL;

for (i = 0; i < limit; i++) {
if (i == 0)
p = non_null();
else
*p = something();
}

Smatch doesn't handle loops correctly. (I know how to fix this but I've
never gotten around to it because it would make Smatch slow)...

So instead of that maybe I would do a hack to silence this type of
warning. Not sure what...

drivers/usb/gadget/udc/amd5536udc_pci.c:61 udc_pci_remove() warn: pointer dereferenced without being set '&udc->gadget'
This one is interesting. Seems like a real bug.

drivers/mtd/ubi/block.c:391 ubiblock_create() warn: pointer dereferenced without being set 'dev->gd'

This one too. So maybe we could make this a separate warning where
NULL dereferences happen on error paths. Or maybe when they happen in
printks.

So there are ways to take this first draft and massage it and get
fewer false positives, by filtering false positives or taking things
which work and creating new checks instead.

Anyway, results attached.

regards,
dan carpenter

[Dan Carpenter]

On Sat, Apr 22, 2023 at 10:48:32PM +0300, Dan Carpenter wrote:
> The warnings from this are quite promising.

Well... The results are over 90% false positives. But there are still
probably 15 bugs in there. The printk warning trick would probably
work.

regards,
dan carpenter

[Dongliang Mu]

Hi Dan,

thanks for your efforts. After finishing the current task list, we can
first ask senior students to check this result quickly and then assign
highly-to-be True Positive to students.

BTW, do you have any plans to improve the code readability, directory
orgranization, documentation etc. of Smatch? It's hard even for senior
students to start with.

[Dan Carpenter]

On Sun, Apr 23, 2023 at 10:33:29AM +0800, Dongliang Mu wrote:
> BTW, do you have any plans to improve the code readability, directory
> orgranization, documentation etc. of Smatch? It's hard even for senior
> students to start with.

I have created some documentation. Read the following blogs in order:

https://staticthinking.wordpress.com/2023/04/24/smatch-data-types/
https://staticthinking.wordpress.com/2023/04/25/first-smatch-check/
https://staticthinking.wordpress.com/2023/04/25/merging-states/
https://staticthinking.wordpress.com/2023/05/02/the-cross-function-db/
https://staticthinking.wordpress.com/2023/05/02/the-param-key-api/
https://staticthinking.wordpress.com/2023/05/02/smatch-hooks-and-modules/
https://staticthinking.wordpress.com/2023/05/02/debugging-smatch-checks/

Email the sma...@vger.kernel.org mailing list with any questions.

regards,
dan carpenter