re: Input: joydev - validate axis/button maps before clobbering current ones

re: Input: joydev - validate axis/button maps before clobbering current ones

[Dan Carpenter]

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

Re: Input: joydev - validate axis/button maps before clobbering current ones

[Stephen Kitt]

[-- Attachment #1: Type: text/plain, Size: 1715 bytes --]

Hello Dan,

On Tue, 6 Oct 2015 21:51:55 +0300, Dan Carpenter <dan.carpenter@oracle.com>
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.

Regards,

Stephen

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: Input: joydev - validate axis/button maps before clobbering current ones

[Stephen Kitt]

[-- Attachment #1: Type: text/plain, Size: 1975 bytes --]

On Tue, 6 Oct 2015 22:57:26 +0200, Stephen Kitt <steve@sk2.org> wrote:
> On Tue, 6 Oct 2015 21:51:55 +0300, Dan Carpenter <dan.carpenter@oracle.com>
> 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.

Regards,

Stephen

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: Input: joydev - validate axis/button maps before clobbering current ones

[Javier Martinez Canillas]

Hello Stephen,

On 10/06/2015 11:01 PM, Stephen Kitt wrote:
> On Tue, 6 Oct 2015 22:57:26 +0200, Stephen Kitt <steve@sk2.org> wrote:
>> On Tue, 6 Oct 2015 21:51:55 +0300, Dan Carpenter <dan.carpenter@oracle.com>
>> 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@osg.samsung.com>
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@osg.samsung.com>
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

Re: Input: joydev - validate axis/button maps before clobbering current ones

[Dmitry Torokhov]

On Tue, Oct 06, 2015 at 11:49:41PM +0200, Javier Martinez Canillas wrote:
> Hello Stephen,
> 
> On 10/06/2015 11:01 PM, Stephen Kitt wrote:
> > On Tue, 6 Oct 2015 22:57:26 +0200, Stephen Kitt <steve@sk2.org> wrote:
> >> On Tue, 6 Oct 2015 21:51:55 +0300, Dan Carpenter <dan.carpenter@oracle.com>
> >> 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.

The original patch is buried under a merge so I'll just apply this one
without squashing.

Thanks.

-- 
Dmitry

Re: Input: joydev - validate axis/button maps before clobbering current ones

[Javier Martinez Canillas]

Hello Dmitry,

On 10/07/2015 12:49 AM, Dmitry Torokhov wrote:
> On Tue, Oct 06, 2015 at 11:49:41PM +0200, Javier Martinez Canillas wrote:
>> Hello Stephen,
>>
>> On 10/06/2015 11:01 PM, Stephen Kitt wrote:
>>> On Tue, 6 Oct 2015 22:57:26 +0200, Stephen Kitt <steve@sk2.org> wrote:
>>>> On Tue, 6 Oct 2015 21:51:55 +0300, Dan Carpenter <dan.carpenter@oracle.com>
>>>> 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.
> 
> 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,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

Re: Input: joydev - validate axis/button maps before clobbering current ones

[Dan Carpenter]

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

On Tue, Oct 06, 2015 at 10:57:26PM +0200, Stephen Kitt wrote:
> Hello Dan,
> 
> On Tue, 6 Oct 2015 21:51:55 +0300, Dan Carpenter <dan.carpenter@oracle.com>
> 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.
> 
> Regards,
> 
> Stephen

Re: Input: joydev - validate axis/button maps before clobbering current ones

[Javier Martinez Canillas]

Hello Dan,

On 10/07/2015 07:46 AM, Dan Carpenter wrote:
> 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
>

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:

https://git.kernel.org/cgit/linux/kernel/git/dtor/input.git/commit/?h=next&id=5b21e3c740b770fb2548a5a8ea66e544d114d0a8

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

Re: Input: joydev - validate axis/button maps before clobbering current ones

[Dan Carpenter]

Fantastic.  Thanks!

regards,
dan carpenter