diff mbox series

[(resend)] Input: MT - limit max slots

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

Commit Message

Tetsuo Handa July 29, 2024, 12:51 p.m. UTC
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(+)

Comments

Greg KH July 29, 2024, 1:05 p.m. UTC | #1
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
Tetsuo Handa July 29, 2024, 1:15 p.m. UTC | #2
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
Greg KH July 29, 2024, 2:28 p.m. UTC | #3
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.
Dmitry Torokhov July 29, 2024, 3:57 p.m. UTC | #4
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.
Linus Torvalds July 29, 2024, 5:43 p.m. UTC | #5
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
Dmitry Torokhov July 29, 2024, 5:59 p.m. UTC | #6
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.
Linus Torvalds July 29, 2024, 6:16 p.m. UTC | #7
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
Dmitry Torokhov July 29, 2024, 6:35 p.m. UTC | #8
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.
Linus Torvalds July 29, 2024, 6:41 p.m. UTC | #9
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
Dmitry Torokhov July 29, 2024, 7:12 p.m. UTC | #10
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.
Linus Torvalds July 29, 2024, 7:27 p.m. UTC | #11
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
Dmitry Torokhov July 29, 2024, 8 p.m. UTC | #12
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.
Linus Torvalds July 29, 2024, 8:14 p.m. UTC | #13
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
Dmitry Torokhov July 29, 2024, 11:17 p.m. UTC | #14
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.
Tetsuo Handa July 30, 2024, 5:38 a.m. UTC | #15
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...
Dmitry Torokhov July 30, 2024, 9:52 p.m. UTC | #16
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 mbox series

Patch

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)