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

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

[Tetsuo Handa]

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(+)

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)
-- 
2.43.5

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

[Greg Kroah-Hartman]

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

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

[Tetsuo Handa]

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

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

[Greg Kroah-Hartman]

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.

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

[Dmitry Torokhov]

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.

-- 
Dmitry

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

[Linus Torvalds]

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

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

[Dmitry Torokhov]

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.

-- 
Dmitry

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

[Linus Torvalds]

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

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

[Dmitry Torokhov]

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.

-- 
Dmitry

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

[Linus Torvalds]

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

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

[Dmitry Torokhov]

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.

-- 
Dmitry

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

[Linus Torvalds]

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

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

[Dmitry Torokhov]

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.

-- 
Dmitry

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

[Linus Torvalds]

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

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

[Dmitry Torokhov]

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.

-- 
Dmitry

---8<------------------8<------------------8<---------------

Input: uinput - reject requests with unreasonable number of slots

From: Dmitry Torokhov <dmitry.torokhov@gmail.com>

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 <penguin-kernel@I-love.SAKURA.ne.jp>
Reported-by: syzbot <syzbot+0122fa359a69694395d5@syzkaller.appspotmail.com>
Closes: https://syzkaller.appspot.com/bug?extid=0122fa359a69694395d5
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/input/misc/uinput.c |   14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/input/misc/uinput.c b/drivers/input/misc/uinput.c
index d23f3225b00f..ea3d34e62eb2 100644
--- a/drivers/input/misc/uinput.c
+++ b/drivers/input/misc/uinput.c
@@ -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;
 }
 

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

[Tetsuo Handa]

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...

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

[Dmitry Torokhov]

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.

-- 
Dmitry