Input: joydev - validate axis/button maps before clobbering current ones
Subsystems (2)
Hello Stephen Kitt,
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.
451 }
452
453 for (i = 0; i < joydev->nabs; i++) {
454 if (abspam[i] > ABS_MAX) {
455 retval = -EINVAL;
456 goto out;
457 }
458 }
459
460 memcpy(joydev->abspam, abspam, len);
461
462 for (i = 0; i < joydev->nabs; i++)
463 joydev->absmap[joydev->abspam[i]] = i;
464
465 out:
466 kfree(abspam);
467 return retval;
468 }regards,
dan carpenter
Hello Dan,
Quoted 23 lines · Dan Carpenter
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:466joydev_handle_JSIOCSAXMAP()
error: 'abspam' dereferencing possibleERR_PTR()
drivers/input/joydev.c
437 static int joydev_handle_JSIOCSAXMAP(struct joydev *joydev,
438 void__user*argp,size_tlen)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;
Quoted 5 lines
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.
Regards,
Stephen
Quoted 35 lines · Stephen Kitt
Quoted 23 lines · Dan Carpenter
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:466joydev_handle_JSIOCSAXMAP()
error: 'abspam' dereferencing possibleERR_PTR()
drivers/input/joydev.c
437 static int joydev_handle_JSIOCSAXMAP(struct joydev *joydev,
438 void__user*argp,size_tlen) 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;Quoted 5 lines
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
Quoted 8 lines
/* 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()andPTR_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.
Regards,
Stephen
Hello Stephen,
Quoted 41 lines · Stephen Kitt
Quoted 40 lines · Stephen Kitt
Quoted 23 lines · Dan Carpenter
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:466joydev_handle_JSIOCSAXMAP()
error: 'abspam' dereferencing possibleERR_PTR()
drivers/input/joydev.c
437 static int joydev_handle_JSIOCSAXMAP(struct joydev *joydev,
438 void__user*argp,size_tlen) 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;Quoted 5 lines
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;
Quoted 7 lines
Quoted 3 lines
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()andPTR_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.
Quoted 4 lines
Regards,
Stephen
Best regards,
Signature · 5 lines
Javier Martinez Canillas Open Source Group Samsung Research America [0]:
Quoted 1 line
From
6b01facd81655276ac9a595d0515b37d9c451d66Mon Sep 17 00:00:00 2001
From: Javier Martinez Canillas <[email protected]>
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.
memory from user-space")
--- drivers/input/joydev.c | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-)
drivers/input/joydev.c +4 -8
@@ -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 ) {
Signature · 1 line
2.4.3
Dmitry Torokhov
maintainerQuoted 52 lines · Javier Martinez Canillas
Hello Stephen,
Quoted 41 lines · Stephen Kitt
Quoted 40 lines · Stephen Kitt
Quoted 23 lines · Dan Carpenter
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:466joydev_handle_JSIOCSAXMAP()
error: 'abspam' dereferencing possibleERR_PTR()
drivers/input/joydev.c
437 static int joydev_handle_JSIOCSAXMAP(struct joydev *joydev,
438 void__user*argp,size_tlen) 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;Quoted 5 lines
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;Quoted 7 lines
Quoted 3 lines
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()andPTR_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.
Quoted 8 lines
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.
The original patch is buried under a merge so I'll just apply this one without squashing.
Thanks.
Signature · 1 line
Dmitry
Hello Dmitry,
Quoted 58 lines · Dmitry Torokhov
Quoted 57 lines · Javier Martinez Canillas
Hello Stephen,
Quoted 41 lines · Stephen Kitt
Quoted 40 lines · Stephen Kitt
Quoted 23 lines · Dan Carpenter
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:466joydev_handle_JSIOCSAXMAP()
error: 'abspam' dereferencing possibleERR_PTR()
drivers/input/joydev.c
437 static int joydev_handle_JSIOCSAXMAP(struct joydev *joydev,
438 void__user*argp,size_tlen) 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;Quoted 5 lines
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;Quoted 7 lines
Quoted 3 lines
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()andPTR_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].
Quoted 9 lines
Quoted 3 lines
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.
The original patch is buried under a merge so I'll just apply this one without squashing.
Thanks.
Ok, I just posted to the list as a proper patch then and also added Dan's Reported-by tag. Again, sorry for the issue.
Best regards,
Signature · 3 lines
Javier Martinez Canillas Open Source Group Samsung Research America
Oh whoops, I sent this to the wrong person. Javier, you introduced a bug with 5702222c9a7a ('Input: joydev - use memdup_user() to duplicate memory from user-space')
regards,
dan carpenter
Quoted 49 lines · Stephen Kitt
Hello Dan,
Quoted 23 lines · Dan Carpenter
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:466joydev_handle_JSIOCSAXMAP()
error: 'abspam' dereferencing possibleERR_PTR()
drivers/input/joydev.c
437 static int joydev_handle_JSIOCSAXMAP(struct joydev *joydev,
438 void__user*argp,size_tlen)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;Quoted 5 lines
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()andPTR_ERR()stuff is coming from.Regards,
Stephen
Hello Dan,
Quoted 7 lines · Dan Carpenter
Oh whoops, I sent this to the wrong person. Javier, you introduced a bug with
5702222c9a7a('Input: joydev - usememdup_user()to duplicate memory from user-space')regards,
dan carpenter
Yes, thanks for reporting it but I've already mentioned in this thread that I posted a fix for it and is already in Dmitry tree:
Best regards,
Signature · 3 lines
Javier Martinez Canillas Open Source Group Samsung Research America
Fantastic. Thanks!
regards,
dan carpenter