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 570c9a7a ('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=5b21e3c740b770fb2548a5a8ea66e544d114d0a8

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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 570c9a7a ('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 
> 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


--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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  wrote:
>> On Tue, 6 Oct 2015 21:51:55 +0300, Dan Carpenter 
>> 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 
Date: Tue, 6 Oct 2015 23:29:06 +0200
Subject: [PATCH 1/1] Input: joydev - fix possible ERR_PTR() dereferencing

Commit 570c9a7a ("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 
Fixes: 570c9a7a ("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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Stephen Kitt]

On Tue, 6 Oct 2015 22:57:26 +0200, Stephen Kitt  wrote:
> On Tue, 6 Oct 2015 21:51:55 +0300, Dan Carpenter 
> 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


pgpx2xg14le6t.pgp
Description: OpenPGP digital signature

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

[Stephen Kitt]

Hello Dan,

On Tue, 6 Oct 2015 21:51:55 +0300, Dan Carpenter 
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


pgprWuabmQyRg.pgp
Description: OpenPGP digital signature

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  wrote:
> >> On Tue, 6 Oct 2015 21:51:55 +0300, Dan Carpenter 
> >> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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  wrote:
 On Tue, 6 Oct 2015 21:51:55 +0300, Dan Carpenter 
 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
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html