Hello Stephen,
On 10/06/2015 11:01 PM, Stephen Kitt wrote:
> On Tue, 6 Oct 2015 22:57:26 +0200, Stephen Kitt <steve@xxxxxxx> wrote:
>> On Tue, 6 Oct 2015 21:51:55 +0300, Dan Carpenter <dan.carpenter@xxxxxxxxxx>
>> wrote:
>>> The patch 999b874f4aa3: "Input: joydev - validate axis/button maps
>>> before clobbering current ones" from Aug 25, 2009, leads to the
>>> following static checker warning:
>>>
>>> drivers/input/joydev.c:466 joydev_handle_JSIOCSAXMAP()
>>> error: 'abspam' dereferencing possible ERR_PTR()
>>>
>>> drivers/input/joydev.c
>>> 437 static int joydev_handle_JSIOCSAXMAP(struct joydev *joydev,
>>> 438 void __user *argp, size_t
>>> len) 439 {
>>> 440 __u8 *abspam;
>>> 441 int i;
>>> 442 int retval = 0;
>>> 443
>>> 444 len = min(len, sizeof(joydev->abspam));
>>> 445
>>> 446 /* Validate the map. */
>>> 447 abspam = memdup_user(argp, len);
>>> 448 if (IS_ERR(abspam)) {
>>> 449 retval = PTR_ERR(abspam);
>>> 450 goto out;
>>>
>>> out labels are error prone. It's safer to return directly.
>>>
>>> https://plus.google.com/106378716002406849458/posts/dnanfhQ4mHQ
>>>
>>> joydev_handle_JSIOCSBTNMAP() has the same issue.
>>
>> Perhaps I'm missing something here, but that's not the code I wrote, nor is
>> it the code that's currently in the kernel. What I have in my copy of the
>> kernel tree is
>>
>> /* Validate the map. */
>> abspam = kmalloc(len, GFP_KERNEL);
>> if (!abspam)
>> return -ENOMEM;
>>
>> which does as you recommend. If you look up the commit you're referring to
>> you'll see that's also the code as I wrote it back in 2009; I'm not sure
>> where your IS_ERR() and PTR_ERR() stuff is coming from.
>
> After further investigation I'm guessing this is
> https://lkml.org/lkml/2015/10/2/370, so cc'ing Javier and Dmitry.
>
It is indeed a bug introduced by my "cleanup" patch, sorry for the mess :(
I double checked when posting the patch but got confused and used the old
error logic. Following is a fixup patch [0].
I don't know if Dmitry prefers to squash with the other patch since it
didn't hit mainline yet or if not I can post it as a proper patch so he
can pick it on his next branch.
> Regards,
>
> Stephen
>
Best regards,
--
Javier Martinez Canillas
Open Source Group
Samsung Research America
[0]:
>From 6b01facd81655276ac9a595d0515b37d9c451d66 Mon Sep 17 00:00:00 2001
From: Javier Martinez Canillas <javier@xxxxxxxxxxxxxxx>
Date: Tue, 6 Oct 2015 23:29:06 +0200
Subject: [PATCH 1/1] Input: joydev - fix possible ERR_PTR() dereferencing
Commit 5702222c9a7a ("Input: joydev - use memdup_user() to duplicate
memory from user-space") changed the kmalloc() and copy_from_user()
with a single call to memdup_user() but wrongly used the same error
path than the old code in which the buffer allocated by kmalloc() was
freed if copy_from_user() failed.
This is of course wrong since if memdup_user() fails, no memory was
allocated and the error in the error-valued pointer should be returned.
Signed-off-by: Javier Martinez Canillas <javier@xxxxxxxxxxxxxxx>
Fixes: 5702222c9a7a ("Input: joydev - use memdup_user() to duplicate
memory from user-space")
---
drivers/input/joydev.c | 12 ++++--------
1 file changed, 4 insertions(+), 8 deletions(-)
diff --git a/drivers/input/joydev.c b/drivers/input/joydev.c
index e3dcd4abae18..5d11fea3c8ec 100644
--- a/drivers/input/joydev.c
+++ b/drivers/input/joydev.c
@@ -445,10 +445,8 @@ static int joydev_handle_JSIOCSAXMAP(struct joydev *joydev,
/* Validate the map. */
abspam = memdup_user(argp, len);
- if (IS_ERR(abspam)) {
- retval = PTR_ERR(abspam);
- goto out;
- }
+ if (IS_ERR(abspam))
+ return PTR_ERR(abspam);
for (i = 0; i < joydev->nabs; i++) {
if (abspam[i] > ABS_MAX) {
@@ -478,10 +476,8 @@ static int joydev_handle_JSIOCSBTNMAP(struct joydev *joydev,
/* Validate the map. */
keypam = memdup_user(argp, len);
- if (IS_ERR(keypam)) {
- retval = PTR_ERR(keypam);
- goto out;
- }
+ if (IS_ERR(keypam))
+ return PTR_ERR(keypam);
for (i = 0; i < joydev->nkey; i++) {
if (keypam[i] > KEY_MAX || keypam[i] < BTN_MISC) {
--
2.4.3
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
