| Message ID | 20210620120030.1513655-1-avlarkin82@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | f8f84af5da9ee04ef1d271528656dac42a090d00 |
| Headers | show |
| Series | Input: joydev - prevent potential write out of bounds in ioctl | expand |
On Sun, Jun 20, 2021 at 5:01 AM Alexander Larkin <avlarkin82@gmail.com> wrote: > > 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. > - 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). > 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"). 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? Linus
On Sun, Jun 20, 2021 at 09:37:47AM -0700, Linus Torvalds wrote: > On Sun, Jun 20, 2021 at 5:01 AM Alexander Larkin <avlarkin82@gmail.com> wrote: > > > > 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. > > > - 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). > > > 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.
On Sun, Jun 20, 2021 at 10:25 PM Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > > 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? I don't have any hardware to test, so please consider that patch of mine more of a "I think something like this" rather than a real patch submission. (I often find it easier to make a patch as a way to just make it very explicit what I think might be the solution, rather than just explaining it in English). So please consider that patch throw-away - it's already gone from my tree. And I don't need authorship for it: finding the problem - like Alexander did - or even just writing a good commit message is worth more than the patch itself, I think. The fix is a one-liner thing, the real work was finding the bug. So if you do want to set me as author, you can add my sign-off, but you can just give credit to Alexander where it really belongs. Linus
I did manual testing with virtual machine (for /dev/input/js0 the driver name is "VirtualBox USB Tablet"). I used program jswap.c from https://bbs.archlinux.org/viewtopic.php?pid=641803#p641803 I compiled both kernel and all modules (including new module with the patch suggested by Linus) and if using updated module: 1. No fail for the poc anymore. 2. Different tests like "./jswap b:0,1 /dev/input/js0" or "./jswap b:4,5 /dev/input/js0"" gives expected result (with seeing the result by cmd "./jswap /dev/input/js0""). 3. Tried to reboot with older kernel (where panic happened) and replayed these few manual tests: results are the same. Both for this older kernel (before fix) for poc.c still seeing fail. 4. For being sure that the kernel module being tested is the same as source code that I'm compiled, I used command "objdump -S joydev.ko" (after decompress of /lib/../joydev.ko.xz). I noted one other minor issue when used jswap. For some reason jswap expected that ioctl JSIOCGBTNMAP returns 0 on success (because there is separate JSIOCGBUTTONS for getting amount of buttons), but instead JSIOCGBTNMAP returns number of buttons too (like JSIOCGBUTTONS is). Is it mistake inside jswap.c and should then keep it as is for joydev.c? So, for making jswap working, first I had to change return status check of JSIOCGBUTTONS inside this test jswap.c program (and I don't know if possible to change return status of JSIOCGBTNMAP to zero on success or of should keep it as is, because many other programs could be broken if fixing success ret status to zero instead of amount of return values). Dmitry, what do you think of this? If no other tests required, maybe let's commit patch that is suggested by Linus. Dmitry, could you please continue with this? Thank you.
Also I did userspace test (that shows how kernel overwrites (out of array bound) the userspace):
1. The buttons is "__u16 buttons[5]" in userspace,
2. buttons[5] = 1;
3. ioctl(fd, JSIOCGBTNMAP, buttons)
4. printf("new %i\n", buttons[5]),
and the output is "new 0", so the value is being overwritten by kernel (so 1024 bytes copied
to 10 bytes buffer).
It looks like I don't understand what is the expected value of the _IOC_SIZE(cmd),
why not 10 for this read ioctl example? Is it possible to make this ioctl safe, so
it doesn't copy more data than user can handle?
I'm still studying "git send-email", so the first intro part of prev msg deleted, sorry, again:
Continuying my previous message, the JSIOCGBTNMAP always returns 1024 return code,
but not "amount of buttons" like I said before
(that is probably the size of the keymap that is _u16 keymap[KEY_MAX - BTN_MISC + 1] ).
Is it correct?
Reading the line of kernel joydev.c
579 len = min_t(size_t, _IOC_SIZE(cmd), sizeof(joydev->keypam)),
, why the min is always sizeof(joydev->keypam) ?
If I try to call from the userspace
ioctl(fd, JSIOCGBTNMAP, buttons)
where the buttons is "__u16 buttons[5]", then still I get 1024.
Also I did userspace test (that shows how kernel overwrites (out of array bound) the userspace):
1. The buttons is "__u16 buttons[5]" in userspace,
2. buttons[5] = 1;
3. ioctl(fd, JSIOCGBTNMAP, buttons)
4. printf("new %i\n", buttons[5]),
and the output is "new 0", so the value is being overwritten by kernel (so 1024 bytes copied
to 10 bytes buffer).
It looks like I don't understand what is the expected value of the _IOC_SIZE(cmd),
why not 10 for this read ioctl example? Is it possible to make this ioctl safe, so
it doesn't copy more data than user can handle?
Hi Alexander, On Tue, Jun 22, 2021 at 12:32:15AM +0300, Alexander Larkin wrote: > I'm still studying "git send-email", so the first intro part of prev msg deleted, sorry, again: > > Continuying my previous message, the JSIOCGBTNMAP always returns 1024 return code, > but not "amount of buttons" like I said before > (that is probably the size of the keymap that is _u16 keymap[KEY_MAX - BTN_MISC + 1] ). > Is it correct? > Reading the line of kernel joydev.c > 579 len = min_t(size_t, _IOC_SIZE(cmd), sizeof(joydev->keypam)), > , why the min is always sizeof(joydev->keypam) ? > If I try to call from the userspace > ioctl(fd, JSIOCGBTNMAP, buttons) > where the buttons is "__u16 buttons[5]", then still I get 1024. > > Also I did userspace test (that shows how kernel overwrites (out of array bound) the userspace): > 1. The buttons is "__u16 buttons[5]" in userspace, > 2. buttons[5] = 1; > 3. ioctl(fd, JSIOCGBTNMAP, buttons) > 4. printf("new %i\n", buttons[5]), > and the output is "new 0", so the value is being overwritten by kernel (so 1024 bytes copied > to 10 bytes buffer). > > It looks like I don't understand what is the expected value of the _IOC_SIZE(cmd), > why not 10 for this read ioctl example? Is it possible to make this ioctl safe, so > it doesn't copy more data than user can handle? The definition of JSIOCGBTNMAP is: #define JSIOCGBTNMAP _IOR('j', 0x34, __u16[KEY_MAX - BTN_MISC + 1]) so the size is encoded in the definition and userspace is supposed to supply buffer of appropriate size. In your examples you are essentially are lying to the kernel and say that the buffer size is KEY_MAX - BTN_MISC + 1 words while in reality you only supply 10 bytes (5 words). Maybe we should have defined (long time ago) #define JSIOCGBTNMAP_LEN(len) _IOC(_IOC_READ, 'j', 0x43, len) to allow userspace better control over the buffer, but I consider joydev interface obsolete and all new clients should be using evdev to get events from input devices. so I am not sure if there is any benefit in fixing joydev ioctls. Thanks.
Hi, On 6/20/21 3:00 PM, Alexander Larkin wrote: > 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. > Since the resulted array of values inited by default with some > good values, the fix is to ignore out of bounds values and > just to use only correct input values by user. > Originally detected by Murray with this simple poc > (If you run the following as an unprivileged user on a default install > it will instantly panic the system: > > int main(void) { > int fd, ret; > unsigned int buffer[10000]; > > fd = open("/dev/input/js0", O_RDONLY); > if (fd == -1) > printf("Error opening file\n"); > > ret = ioctl(fd, JSIOCSBTNMAP & ~IOCSIZE_MASK, &buffer); > printf("%d\n", ret); > } > > Fixes: 182d679b2298 ("Input: joydev - prevent potential read overflow in ioctl") I'm not sure that this is a proper fixes tag. Seems like the bug is in the code since the first commit (1da177e4c3f4). Maybe it's possible to add 2 fixes tags just to notify developers that this bug is older than a 182d679b2298 partial fix. > Reported-by: Murray McAllister <murray.mcallister@gmail.com> > Signed-off-by: Alexander Larkin <avlarkin82@gmail.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 da8963a9f044..1aa067d4a3e8 100644 > --- a/drivers/input/joydev.c > +++ b/drivers/input/joydev.c > @@ -464,7 +464,7 @@ static int joydev_handle_JSIOCSAXMAP(struct joydev *joydev, > > memcpy(joydev->abspam, abspam, len); > > - for (i = 0; i < joydev->nabs; i++) > + for (i = 0; i < len && i < joydev->nabs; i++) > joydev->absmap[joydev->abspam[i]] = i; > > out: > @@ -498,7 +498,7 @@ static int joydev_handle_JSIOCSBTNMAP(struct joydev *joydev, > > 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; > > out: >
On Sat, Jul 03, 2021 at 07:21:58PM +0300, Denis Efremov wrote: > Hi, > > On 6/20/21 3:00 PM, Alexander Larkin wrote: > > 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. > > Since the resulted array of values inited by default with some > > good values, the fix is to ignore out of bounds values and > > just to use only correct input values by user. > > Originally detected by Murray with this simple poc > > (If you run the following as an unprivileged user on a default install > > it will instantly panic the system: > > > > int main(void) { > > int fd, ret; > > unsigned int buffer[10000]; > > > > fd = open("/dev/input/js0", O_RDONLY); > > if (fd == -1) > > printf("Error opening file\n"); > > > > ret = ioctl(fd, JSIOCSBTNMAP & ~IOCSIZE_MASK, &buffer); > > printf("%d\n", ret); > > } > > > > Fixes: 182d679b2298 ("Input: joydev - prevent potential read overflow in ioctl") > > > I'm not sure that this is a proper fixes tag. Seems like the bug is in the > code since the first commit (1da177e4c3f4). Maybe it's possible to add 2 fixes > tags just to notify developers that this bug is older than a 182d679b2298 > partial fix. Normally just setting the fixes tag to my patch would be the correct thing to do. But in this case, I didn't get a CVE for my patch so scripts which determine if a patch is required automatically might get confused? It's not unusual to use two fixes tags so it might be a good idea in this case just to avoid any confusion. regards, dan carpenter
diff --git a/drivers/input/joydev.c b/drivers/input/joydev.c index da8963a9f044..1aa067d4a3e8 100644 --- a/drivers/input/joydev.c +++ b/drivers/input/joydev.c @@ -464,7 +464,7 @@ static int joydev_handle_JSIOCSAXMAP(struct joydev *joydev, memcpy(joydev->abspam, abspam, len); - for (i = 0; i < joydev->nabs; i++) + for (i = 0; i < len && i < joydev->nabs; i++) joydev->absmap[joydev->abspam[i]] = i; out: @@ -498,7 +498,7 @@ static int joydev_handle_JSIOCSBTNMAP(struct joydev *joydev, 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; out:
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. Since the resulted array of values inited by default with some good values, the fix is to ignore out of bounds values and just to use only correct input values by user. Originally detected by Murray with this simple poc (If you run the following as an unprivileged user on a default install it will instantly panic the system: int main(void) { int fd, ret; unsigned int buffer[10000]; fd = open("/dev/input/js0", O_RDONLY); if (fd == -1) printf("Error opening file\n"); ret = ioctl(fd, JSIOCSBTNMAP & ~IOCSIZE_MASK, &buffer); printf("%d\n", ret); } Fixes: 182d679b2298 ("Input: joydev - prevent potential read overflow in ioctl") Reported-by: Murray McAllister <murray.mcallister@gmail.com> Signed-off-by: Alexander Larkin <avlarkin82@gmail.com> --- drivers/input/joydev.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)