| Message ID | a7eb34e0-28cf-4e18-b642-ea8d7959f0c7@I-love.SAKURA.ne.jp (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | 99d3bf5f7377d42f8be60a6b9cb60fb0be34dceb |
| Headers | show |
| Series | [(resend)] Input: MT - limit max slots | expand |
On Mon, Jul 29, 2024 at 09:51:30PM +0900, Tetsuo Handa wrote: > 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. > > Reported-by: syzbot <syzbot+0122fa359a69694395d5@syzkaller.appspotmail.com> > Closes: https://syzkaller.appspot.com/bug?extid=0122fa359a69694395d5 > Suggested-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> > Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> > --- > 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
On 2024/07/29 22:05, Greg Kroah-Hartman wrote: > On Mon, Jul 29, 2024 at 09:51:30PM +0900, Tetsuo Handa wrote: >> 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. >> >> Reported-by: syzbot <syzbot+0122fa359a69694395d5@syzkaller.appspotmail.com> >> Closes: https://syzkaller.appspot.com/bug?extid=0122fa359a69694395d5 >> Suggested-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> >> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> >> --- >> 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? 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/03e8c3f0-bbbf-af37-6f52-67547cbd4cde@I-love.SAKURA.ne.jp 2023-09-03 13:55 https://lkml.kernel.org/r/07350163-de52-a2bf-58bf-88c3d9d8d85b@I-love.SAKURA.ne.jp 2024-05-27 10:35 https://lkml.kernel.org/r/7b7f9cf5-a1de-4e5a-8d30-bb2979309f02@I-love.SAKURA.ne.jp
On Mon, Jul 29, 2024 at 10:15:26PM +0900, Tetsuo Handa wrote: > On 2024/07/29 22:05, Greg Kroah-Hartman wrote: > > On Mon, Jul 29, 2024 at 09:51:30PM +0900, Tetsuo Handa wrote: > >> 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. > >> > >> Reported-by: syzbot <syzbot+0122fa359a69694395d5@syzkaller.appspotmail.com> > >> Closes: https://syzkaller.appspot.com/bug?extid=0122fa359a69694395d5 > >> Suggested-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> > >> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> > >> --- > >> 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? > > 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/03e8c3f0-bbbf-af37-6f52-67547cbd4cde@I-love.SAKURA.ne.jp > 2023-09-03 13:55 https://lkml.kernel.org/r/07350163-de52-a2bf-58bf-88c3d9d8d85b@I-love.SAKURA.ne.jp > 2024-05-27 10:35 https://lkml.kernel.org/r/7b7f9cf5-a1de-4e5a-8d30-bb2979309f02@I-love.SAKURA.ne.jp > Again, it's up to the Input maintainer.
On Mon, Jul 29, 2024 at 04:28:38PM +0200, Greg Kroah-Hartman wrote: > On Mon, Jul 29, 2024 at 10:15:26PM +0900, Tetsuo Handa wrote: > > On 2024/07/29 22:05, Greg Kroah-Hartman wrote: > > > On Mon, Jul 29, 2024 at 09:51:30PM +0900, Tetsuo Handa wrote: > > >> 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. > > >> > > >> Reported-by: syzbot <syzbot+0122fa359a69694395d5@syzkaller.appspotmail.com> > > >> Closes: https://syzkaller.appspot.com/bug?extid=0122fa359a69694395d5 > > >> Suggested-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> > > >> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> > > >> --- > > >> 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? > > > > 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/03e8c3f0-bbbf-af37-6f52-67547cbd4cde@I-love.SAKURA.ne.jp > > 2023-09-03 13:55 https://lkml.kernel.org/r/07350163-de52-a2bf-58bf-88c3d9d8d85b@I-love.SAKURA.ne.jp > > 2024-05-27 10:35 https://lkml.kernel.org/r/7b7f9cf5-a1de-4e5a-8d30-bb2979309f02@I-love.SAKURA.ne.jp > > > > 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.
On Mon, 29 Jul 2024 at 08:57, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > > 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
On Mon, Jul 29, 2024 at 10:43:58AM -0700, Linus Torvalds wrote: > On Mon, 29 Jul 2024 at 08:57, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > > > > 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? > > 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. > > 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.
On Mon, 29 Jul 2024 at 10:59, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > > 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? 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
On Mon, Jul 29, 2024 at 11:16:02AM -0700, Linus Torvalds wrote: > On Mon, 29 Jul 2024 at 10:59, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > > > > 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? > > 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"? > > 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.
On Mon, 29 Jul 2024 at 11:35, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > > 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. > 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
On Mon, Jul 29, 2024 at 11:41:59AM -0700, Linus Torvalds wrote: > On Mon, 29 Jul 2024 at 11:35, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > > > > 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. > > > > 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". 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.
On Mon, 29 Jul 2024 at 12:12, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > > 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. 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
On Mon, Jul 29, 2024 at 12:27:05PM -0700, Linus Torvalds wrote: > On Mon, 29 Jul 2024 at 12:12, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > > > > 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. > > 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.
On Mon, 29 Jul 2024 at 13:00, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > > 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. 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
On Mon, Jul 29, 2024 at 01:14:36PM -0700, Linus Torvalds wrote: > On Mon, 29 Jul 2024 at 13:00, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > > > > 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. > > 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. > > 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.
On 2024/07/30 2:59, Dmitry Torokhov wrote: > 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. 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/6d878e01-6c2f-8766-2578-c95030442369@I-love.SAKURA.ne.jp Lucky thing is that the uinput interface is for only the "root" user...
Hi Tetsuo, On Tue, Jul 30, 2024 at 02:38:19PM +0900, Tetsuo Handa wrote: > On 2024/07/30 2:59, Dmitry Torokhov wrote: > > 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. > > 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/6d878e01-6c2f-8766-2578-c95030442369@I-love.SAKURA.ne.jp I'll happily take the change converting that to array_size(). > > 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.
diff --git a/drivers/input/input-mt.c b/drivers/input/input-mt.c index 14b53dac1253..6b04a674f832 100644 --- a/drivers/input/input-mt.c +++ b/drivers/input/input-mt.c @@ -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)
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. Reported-by: syzbot <syzbot+0122fa359a69694395d5@syzkaller.appspotmail.com> Closes: https://syzkaller.appspot.com/bug?extid=0122fa359a69694395d5 Suggested-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> --- 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(+)