Re: [PATCH] Input: joydev - prevent potential write out of bounds in ioctl

On Sun, Jun 20, 2021 at 09:37:47AM -0700, Linus Torvalds wrote:

On Sun, Jun 20, 2021 at 5:01 AM Alexander Larkin [off-list ref] wrote:

quoted

    The problem is that the check of user input values that is just
    before the fixed line of code is for the part of first values
    (before len or before len/2), but then the usage of all the values
    including i >= len (or i >= len/2) could be.

No, I think the problem is simpler than that.

quoted

-       for (i = 0; i < joydev->nabs; i++)
+       for (i = 0; i < len && i < joydev->nabs; i++)
                joydev->absmap[joydev->abspam[i]] = i;

This part is unnecessary - all values of "joydev->abspam[i]" have been
validated (either they are the old ones, or the new ones that we just
validated).

quoted

        memcpy(joydev->keypam, keypam, len);

-       for (i = 0; i < joydev->nkey; i++)
+       for (i = 0; i < (len / 2) && i < joydev->nkey; i++)
                joydev->keymap[keypam[i] - BTN_MISC] = i;

The problem here is not that we walk past "len/2", but that the code
*should* have used

        joydev->keymap[joydev->keypam[i] - BTN_MISC] = i;

(note the "keypam[1]" vs "joydev->keypam[i]").

And the reason it *should* walk the whole "joydev->nkey" is that if
there are later cases with the same keypam value, the later ones
should override the previous ones (well, that "should" is more a
"traditionally have").

Yes, we can discuss whether "short" ioctl should clear out the part of
map that is not supplied by the call, but given that I consider joydev
legacy my preference would be to leave this as it was.

So I think the right patch is this one-liner

  diff --git a/drivers/input/joydev.c b/drivers/input/joydev.c
  index da8963a9f044..947d440a3be6 100644
  --- a/drivers/input/joydev.c
  +++ b/drivers/input/joydev.c
  @@ -499,7 +499,7 @@ static int joydev_handle_JSIOCSBTNMAP(struct
joydev *joydev,
        memcpy(joydev->keypam, keypam, len);

        for (i = 0; i < joydev->nkey; i++)
  -             joydev->keymap[keypam[i] - BTN_MISC] = i;
  +             joydev->keymap[joydev->keypam[i] - BTN_MISC] = i;

    out:
        kfree(keypam);

(whitespace-damaged, I would like Dmitry to think about it rather than
apply this mindlessly.

Dmitry?

Yes, this makes sense to me and it is safe as joydev->keypam is
guaranteed to be the right size.

Are you going to reformat this and resend or should I?


Thanks.

-- 
Dmitry