Input: MT - limit max slots
Subsystems (3)
PATCH 1/1 Input: MT - limit max slots
syzbot is reporting too large allocation at input_mt_init_slots(), for num_slots is supplied from userspace using ioctl(UI_DEV_CREATE).
Since nobody knows possible max slots, this patch chose 1024.
---
This patch was tested in linux-next between next-20240611 and next-20240729 via my tree. Who can take this patch? If nobody can, I will send to Linus.
drivers/input/input-mt.c | 3 +++ 1 file changed, 3 insertions(+)
drivers/input/input-mt.c +3 -0
@@ -46,6 +46,9 @@ int input_mt_init_slots(struct input_dev *dev, unsigned int num_slots,return 0 ;if (mt)return mt->num_slots != num_slots ? -EINVAL :0 ; +/* Arbitrary limit for avoiding too large memory allocation. */ +if (num_slots >1024 ) +return -EINVAL ; mt =kzalloc (struct_size (mt, slots, num_slots),GFP_KERNEL );if (!mt)
Signature · 1 line
2.43.5
Quoted 5 lines · Tetsuo Handa
syzbot is reporting too large allocation at
input_mt_init_slots(), fornum_slotsis supplied from userspace usingioctl(UI_DEV_CREATE).Since nobody knows possible max slots, this patch chose 1024.
Quoted 7 lines
Reported-by syzbot <[email protected]>Suggested-by Dmitry Torokhov <[email protected]>Signed-off-by Tetsuo Handa <[email protected]>---
This patch was tested in linux-next between next-20240611 and next-20240729 via my tree. Who can take this patch? If nobody can, I will send to Linus.
What is wrong with the normal input maintainer and tree? Why not send it there?
thanks,
greg k-h
Quoted 14 lines · Greg Kroah-Hartman
Quoted 5 lines · Tetsuo Handa
syzbot is reporting too large allocation at
input_mt_init_slots(), fornum_slotsis supplied from userspace usingioctl(UI_DEV_CREATE).Since nobody knows possible max slots, this patch chose 1024.
Quoted 7 lines
Reported-by syzbot <[email protected]>Suggested-by Dmitry Torokhov <[email protected]>Signed-off-by Tetsuo Handa <[email protected]>---
This patch was tested in linux-next between next-20240611 and next-20240729 via my tree. Who can take this patch? If nobody can, I will send to Linus.
Quoted 2 lines
What is wrong with the normal input maintainer and tree? Why not send it there?
I don't know why. I couldn't get further response from Dmitry Torokhov. Who can make final decision and what tree is used?
e.g.
2022-11-23 0:28 https://lkml.kernel.org/r/[email protected]
2023-09-03 13:55 https://lkml.kernel.org/r/[email protected]
2024-05-27 10:35 https://lkml.kernel.org/r/[email protected]
Quoted 18 lines · Tetsuo Handa
Quoted 14 lines · Greg Kroah-Hartman
Quoted 5 lines · Tetsuo Handa
syzbot is reporting too large allocation at
input_mt_init_slots(), fornum_slotsis supplied from userspace usingioctl(UI_DEV_CREATE).Since nobody knows possible max slots, this patch chose 1024.
Quoted 7 lines
Reported-by syzbot <[email protected]>Suggested-by Dmitry Torokhov <[email protected]>Signed-off-by Tetsuo Handa <[email protected]>---
This patch was tested in linux-next between next-20240611 and next-20240729 via my tree. Who can take this patch? If nobody can, I will send to Linus.Quoted 2 lines
What is wrong with the normal input maintainer and tree? Why not send it there?
Quoted 8 lines
I don't know why. I couldn't get further response from Dmitry Torokhov. Who can make final decision and what tree is used?
e.g.
2022-11-23 0:28 https://lkml.kernel.org/r/[email protected]
2023-09-03 13:55 https://lkml.kernel.org/r/[email protected]
2024-05-27 10:35 https://lkml.kernel.org/r/[email protected]
Again, it's up to the Input maintainer.
Dmitry Torokhov
maintainerQuoted 22 lines · Greg Kroah-Hartman
Quoted 21 lines · Tetsuo Handa
Quoted 14 lines · Greg Kroah-Hartman
Quoted 5 lines · Tetsuo Handa
syzbot is reporting too large allocation at
input_mt_init_slots(), fornum_slotsis supplied from userspace usingioctl(UI_DEV_CREATE).Since nobody knows possible max slots, this patch chose 1024.
Quoted 7 lines
Reported-by syzbot <[email protected]>Suggested-by Dmitry Torokhov <[email protected]>Signed-off-by Tetsuo Handa <[email protected]>---
This patch was tested in linux-next between next-20240611 and next-20240729 via my tree. Who can take this patch? If nobody can, I will send to Linus.Quoted 2 lines
What is wrong with the normal input maintainer and tree? Why not send it there?
I don't know why. I couldn't get further response from Dmitry Torokhov. Who can make final decision and what tree is used?
Quoted 7 lines
Quoted 5 lines
e.g.
2022-11-23 0:28 https://lkml.kernel.org/r/[email protected]
2023-09-03 13:55 https://lkml.kernel.org/r/[email protected]
2024-05-27 10:35 https://lkml.kernel.org/r/[email protected]Again, it's up to the Input maintainer.
Sorry, I meant to respond to this and it got lost in my mailbox. To be honest I really dislike all this extra patching for the sake of syzkaller. Syzkaller is a tool and ideally we should not modify random places in the kernel just because it decided to [ab]use one mechanism that is also used for other purposes.
Now, to the issue at hand. I believe there are 2 classes of warnings: a true warning that is emitted for clearly unexpected situation that was caused by a bug or an invariant violation somewhere. Those we definitely want syzkaller to report so that we could identify the root cause and fix it.
iThe other types of warnings, such as the warning in the memory allocation case, are warnings of convenience. We have them so that we do not need to annotate all memory allocation checks with custom messages. Instead we just rely on k*alloc() and friends to give us a spew when they can't allocate memory. These allocation failures are expected and typically are handled (and if they are not handled we'll get an oops a moment later). We really do not need syzkaller to trip on those.
Can we change k*alloc() to stop triggering panic on warning when we run with syzkaller?
Thanks.
Signature · 1 line
Dmitry
Quoted 3 lines · Dmitry Torokhov
iThe other types of warnings, such as the warning in the memory allocation case, are warnings of convenience.
No.
They are WARNINGS OF BUGS.
They are basically warning that the code seems to allow arbitrary allocation sizes.
So apparently you've been sitting on this problem for two years because you didn't understand that you had a bug, and thought the warning was some "convenience thing".
I'll just apply it directly. Don't do this again.
Linus
Dmitry Torokhov
maintainerQuoted 11 lines · Linus Torvalds
Quoted 3 lines · Dmitry Torokhov
iThe other types of warnings, such as the warning in the memory allocation case, are warnings of convenience.
No.
They are WARNINGS OF BUGS.
They are basically warning that the code seems to allow arbitrary allocation sizes.
No, this is decidedly not a bug. As with any other resource, if it is available it can be allocated and if it is not available the code should handle the failures.
Can I write a gigabyte of data to disk? Terabyte? Is petabyte too much? What if I don't have enough physical disk. Do we "fix" write() not to take size_t length?
Quoted 4 lines
So apparently you've been sitting on this problem for two years because you didn't understand that you had a bug, and thought the warning was some "convenience thing".
Yes, it is a convenience thing. Same as some code wanting to allocate 2 or 4 pages and sometimes failing when the system is under load.
Quoted 2 lines
I'll just apply it directly. Don't do this again.
Please do not. Or you will have to patch it again when we will still see the same allocation failures because someone requested an input device with "too many" slots (1024 results in 4Mb mt->red table for example).
Just fix malloc/syzkaller not to trigger on benign memory allocation hickups. They are normal.
Thanks.
Signature · 1 line
Dmitry
Quoted 4 lines · Dmitry Torokhov
Can I write a gigabyte of data to disk? Terabyte? Is petabyte too much? What if I don't have enough physical disk. Do we "fix"
write()not to takesize_tlength?
Dmitry, that's *EXACTLY* what we did decades ago.
Your argument is bogus garbage. We do various arbitrary limits exactly to head off problems early.
Linus
Dmitry Torokhov
maintainerQuoted 7 lines · Linus Torvalds
Quoted 4 lines · Dmitry Torokhov
Can I write a gigabyte of data to disk? Terabyte? Is petabyte too much? What if I don't have enough physical disk. Do we "fix"
write()not to takesize_tlength?Dmitry, that's *EXACTLY* what we did decades ago.
What exactly did you do? Limit size of data userspace can request to be written? What is the max allowed size then? Can I stick a warning in the code to complain when it is "too big"?
Quoted 3 lines
Your argument is bogus garbage. We do various arbitrary limits exactly to head off problems early.
So does this mean that we should disallow any and all allocations above 4k because they can potentially fail, depending on the system state? Or maybe we should be resilient and fail gracefully instead?
It would help if you expanded why exactly my argument is a garbage and what the problem is with recognizing that memory is a depletable resource (like a lot of other resources, including storage) and there's never a "completely safe" amount that can be used, so trying to introduce it is futile.
Thanks.
Signature · 1 line
Dmitry
Quoted 4 lines · Dmitry Torokhov
What exactly did you do? Limit size of data userspace can request to be written? What is the max allowed size then? Can I stick a warning in the code to complain when it is "too big"?
Look up MAX_RW_COUNT.
Quoted 3 lines
So does this mean that we should disallow any and all allocations above 4k because they can potentially fail, depending on the system state? Or maybe we should be resilient and fail gracefully instead?
We are resilient and fail gracefully.
But there's very a limit to that.
Dmitry - none of this is at all new. The kernel has a *lot* of practical limits. Many of them actually come from very traditional sources indeed.
Things like NR_OPEN, PATH_MAX, lots of arbitrary limits because arrays don't get to grow too big. Things that are *so* basic that you don't even think about them, because you think they are obvious.
In fact, you should start from the assumption that *EVERYTHING* is limited.
So get off your idiotic high horse. The input layer is not so special that you should say "I can't have any limits".
Linus
Dmitry Torokhov
maintainerQuoted 21 lines · Linus Torvalds
Quoted 4 lines · Dmitry Torokhov
What exactly did you do? Limit size of data userspace can request to be written? What is the max allowed size then? Can I stick a warning in the code to complain when it is "too big"?
Look up
MAX_RW_COUNT.Quoted 3 lines
So does this mean that we should disallow any and all allocations above 4k because they can potentially fail, depending on the system state? Or maybe we should be resilient and fail gracefully instead?
We are resilient and fail gracefully.
But there's very a limit to that.
Dmitry - none of this is at all new. The kernel has a *lot* of practical limits. Many of them actually come from very traditional sources indeed.
Quoted 8 lines
Things like
NR_OPEN,PATH_MAX, lots of arbitrary limits because arrays don't get to grow too big. Things that are *so* basic that you don't even think about them, because you think they are obvious.In fact, you should start from the assumption that *EVERYTHING* is limited.
So get off your idiotic high horse. The input layer is not so special that you should say "I can't have any limits".
OK, if you want to have limits be it. You probably want to lower from 1024 to 128 or something, because with 1024 slots the structure will be larger than one page and like I said mt->red table will be 4Mb.
This still will not affect any real users, and will not solve syzkaller issue as it will continue tripping on various memory allocations that we are actually prepared to handle.
Thanks.
Signature · 1 line
Dmitry
Quoted 4 lines · Dmitry Torokhov
OK, if you want to have limits be it. You probably want to lower from 1024 to 128 or something, because with 1024 slots the structure will be larger than one page and like I said
mt->redtable will be 4Mb.
So this is why subsystem maintainers should be involved and helpful. It's hard to know what practical limits are.
That said, a 4MB allocation for some test code is nothing.
And yes, if syzbot hits other cases where the input layer just takes user input without any limit sanity checking, those should be fixed *too*.
"The allocation failed" is not some graceful thing. Large pointless allocations can be very very very expensive *before* they fail, because the VM layer will spend lots of time trying to clean things up.
So a sane limit *UP FRONT* instead of "let's see if this random allocation succeeds" is always the right answer when at all possible.
Linus
Dmitry Torokhov
maintainerQuoted 6 lines · Linus Torvalds
Quoted 4 lines · Dmitry Torokhov
OK, if you want to have limits be it. You probably want to lower from 1024 to 128 or something, because with 1024 slots the structure will be larger than one page and like I said
mt->redtable will be 4Mb.
Quoted 8 lines
So this is why subsystem maintainers should be involved and helpful. It's hard to know what practical limits are.
That said, a 4MB allocation for some test code is nothing.
And yes, if syzbot hits other cases where the input layer just takes user input without any limit sanity checking, those should be fixed *too*.
Hmm, maybe the checks should go into drivers/input/misc/uinput.c which is the only place that allows userspace to create input device instances and drive them rather than into input core logic because all other devices are backed by real hardware.
Thanks.
Signature · 1 line
Dmitry
Quoted 5 lines · Dmitry Torokhov
Hmm, maybe the checks should go into
drivers/input/misc/uinput.cwhich is the only place that allows userspace to create input device instances and drive them rather than into input core logic because all other devices are backed by real hardware.
Ack, that sounds like a good idea, particularly if there is some single location that could validate the input.
uinput_validate_absinfo(), perhaps?
We do end up trying to protect against some forms of bad hardware too when possible, but realistically _that_ kind of protection should be more along the lines of "don't cause security issues".
Linus
Dmitry Torokhov
maintainerQuoted 11 lines · Linus Torvalds
Quoted 5 lines · Dmitry Torokhov
Hmm, maybe the checks should go into
drivers/input/misc/uinput.cwhich is the only place that allows userspace to create input device instances and drive them rather than into input core logic because all other devices are backed by real hardware.Ack, that sounds like a good idea, particularly if there is some single location that could validate the input.
uinput_validate_absinfo(), perhaps?
Yes, I think that's the right place.
Quoted 4 lines
We do end up trying to protect against some forms of bad hardware too when possible, but realistically _that_ kind of protection should be more along the lines of "don't cause security issues".
Most of them have statically defined slot limit. The exception is HID (which can also be fed garbage through uhid) but hid-multitouch uses u8 for maximum number of contacts, so can't go above 256.
Thanks.
Signature · 25 lines
Dmitry ---8<------------------8<------------------8<--------------- Input: uinput - reject requests with unreasonable number of slots From: Dmitry Torokhov <[email protected]> When exercising uinput interface syzkaller may try setting up device with a really large number of slots, which causes memory allocation failure in input_mt_init_slots(). While this allocation failure is handled properly and request is rejected, it results in syzkaller reports. Additionally, such request may put undue burden on the system which will try to free a lot of memory for a bogus request. Fix it by limiting allowed number of slots to 100. This can easily be extended if we see devices that can track more than 100 contacts. Reported-by: Tetsuo Handa <[email protected]> Reported-by: syzbot <[email protected]> Closes: https://syzkaller.appspot.com/bug?extid=0122fa359a69694395d5 Signed-off-by: Dmitry Torokhov <[email protected]> --- drivers/input/misc/uinput.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+)
drivers/input/misc/uinput.c +14 -0
@@ -417,6 +417,20 @@ static int uinput_validate_absinfo(struct input_dev *dev, unsigned int code,return -EINVAL ; } +/* +* Limit number of contacts to a reasonable value (100). This +* ensures that we need less than 2 pages for struct input_mt +* (we are not using in-kernel slot assignment so not going to +* allocate memory for the "red" table), and we should have no +* trouble getting this much memory. +*/ +if (code ==ABS_MT_SLOT && max >99 ) { +printk (KERN_DEBUG +"%s: unreasonably large number of slots requested: %d\n" , +UINPUT_NAME , max); +return -EINVAL ; + } +return 0 ; }
Quoted 6 lines · Dmitry Torokhov
Please do not. Or you will have to patch it again when we will still see the same allocation failures because someone requested an input device with "too many" slots (1024 results in 4Mb
mt->redtable for example).Just fix malloc/syzkaller not to trigger on benign memory allocation hickups. They are normal.
I chose 1024 because as far as I know 4MB is max acceptable size for all environments without triggering too large allocation warning.
You worry about mt->red, but did you notice that syzbot was reporting that memory allocation for mt->red has an integer overflow bug, which can cause out of bounds write or ZERO_SIZE_PTR pointer dereference bug at input_mt_set_matrix() ?
https://lkml.kernel.org/r/[email protected]
Lucky thing is that the uinput interface is for only the "root" user...
Dmitry Torokhov
maintainerHi Tetsuo,
Quoted 8 lines · Tetsuo Handa
Quoted 6 lines · Dmitry Torokhov
Please do not. Or you will have to patch it again when we will still see the same allocation failures because someone requested an input device with "too many" slots (1024 results in 4Mb
mt->redtable for example).Just fix malloc/syzkaller not to trigger on benign memory allocation hickups. They are normal.
Quoted 8 lines
I chose 1024 because as far as I know 4MB is max acceptable size for all environments without triggering too large allocation warning.
You worry about
mt->red, but did you notice that syzbot was reporting that memory allocation formt->redhas an integer overflow bug, which can cause out of bounds write orZERO_SIZE_PTRpointer dereference bug atinput_mt_set_matrix()?
I'll happily take the change converting that to array_size().
Quoted 2 lines
Lucky thing is that the uinput interface is for only the "root" user...
uinput also does not request in-kernel contact tracking so it will not allow hitting that overflow.
Thanks.
Signature · 1 line
Dmitry