Re: [PATCH] Input: joydev - prevent potential read overflow in ioctl

[Dan Carpenter <dan.carpenter@oracle.com>]

On Sat, Jul 03, 2021 at 02:20:13PM +0300, Denis Efremov wrote:
> Hi,
> 
> CVE-2021-3612 was assigned to this patch.
> 
> 
> On 2/17/21 9:10 AM, Dan Carpenter wrote:
> > The problem here is that "len" might be less than "joydev->nabs" so the
> > loops which verfy abspam[i] and keypam[] might read beyond the buffer.
> 
> 
> The added check looks insufficient to me. There are second loops in these
> functions with unpatched "i < joydev->nabs;" and "i < joydev->nkey;" checks.
> 

Thanks, Denis.

You're right.  The fix isn't complete.  We discussed this in a different
thread but I sort of assumed it was dealt with and didn't follow up.  :/

https://lore.kernel.org/lkml/20210620120030.1513655-1-avlarkin82@gmail.com/

regards,
dan carpenter

> > 
> > Fixes: 999b874f4aa3 ("Input: joydev - validate axis/button maps before clobbering current ones")
> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > ---
> >  drivers/input/joydev.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/input/joydev.c b/drivers/input/joydev.c
> > index a2b5fbba2d3b..750f4513fe20 100644
> > --- a/drivers/input/joydev.c
> > +++ b/drivers/input/joydev.c
> > @@ -456,7 +456,7 @@ static int joydev_handle_JSIOCSAXMAP(struct joydev *joydev,
> >  	if (IS_ERR(abspam))
> >  		return PTR_ERR(abspam);
> >  
> > -	for (i = 0; i < joydev->nabs; i++) {
> > +	for (i = 0; i < len && i < joydev->nabs; i++) {
> >  		if (abspam[i] > ABS_MAX) {
> >  			retval = -EINVAL;
> >  			goto out;
> 
>         memcpy(joydev->abspam, abspam, len);
> 
>         for (i = 0; i < joydev->nabs; i++) // <== HERE
>                 joydev->absmap[joydev->abspam[i]] = i;
> 
> 
> > @@ -487,7 +487,7 @@ static int joydev_handle_JSIOCSBTNMAP(struct joydev *joydev,
> >  	if (IS_ERR(keypam))
> >  		return PTR_ERR(keypam);
> >  
> > -	for (i = 0; i < joydev->nkey; i++) {
> > +	for (i = 0; i < (len / 2) && i < joydev->nkey; i++) {
> >  		if (keypam[i] > KEY_MAX || keypam[i] < BTN_MISC) {
> >  			retval = -EINVAL;
> >  			goto out;
> > 
> 
>         memcpy(joydev->keypam, keypam, len);
> 
>         for (i = 0; i < joydev->nkey; i++) // <== HERE
>                 joydev->keymap[keypam[i] - BTN_MISC] = i;
> 
> 
> Also at the beginning of joydev_handle_JSIOCSAXMAP() there is a
> 	len = min(len, sizeof(joydev->abspam));
> 	abspam = memdup_user(argp, len);
> 
> Maybe we can call min(len, joydev->nabs) instead or even min3() and
> use only len in the for loops then? Same for joydev_handle_JSIOCSBTNMAP.
> 
> Thanks,
> Denis