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

[Dmitry Torokhov <dmitry.torokhov@gmail.com>]

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