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: [email protected]
Signed-off-by: Dongliang Mu <[email protected]>
---
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
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: [email protected]
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
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
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?
>
> Dongliang Mu
>
>>> 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: [email protected]
>>>> 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
Vicki
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.
>
>> Dongliang Mu
>>
>>>> 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: [email protected]
>>>>> 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
> Vicki
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: [email protected]
>> 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
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
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
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
On 2023/4/23 03:48, Dan Carpenter wrote:
> 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.
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.
> Anyway, results attached.
>
> regards,
> 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 [email protected] mailing list with any questions.
regards,
dan carpenter