[PATCH] Input: MT - use __GFP_NOWARN allocation at input_mt_init_slots()

[PATCH] Input: MT - use __GFP_NOWARN allocation at input_mt_init_slots()

[Tetsuo Handa]

syzbot is reporting too large allocation at input_mt_init_slots() {1], for
num_slots is supplied from userspace using ioctl(UI_DEV_CREATE).
Also, replace n2 with array_size(), for 32bits variable n2 will overflow
if num_slots >= 65536.

Link: https://syzkaller.appspot.com/bug?extid=0122fa359a69694395d5 [1]
Reported-by: syzbot <syzbot+0122fa359a69694395d5@syzkaller.appspotmail.com>
Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
 drivers/input/input-mt.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/input/input-mt.c b/drivers/input/input-mt.c
index 14b53dac1253..cf74579462ba 100644
--- a/drivers/input/input-mt.c
+++ b/drivers/input/input-mt.c
@@ -47,7 +47,7 @@ int input_mt_init_slots(struct input_dev *dev, unsigned int num_slots,
 	if (mt)
 		return mt->num_slots != num_slots ? -EINVAL : 0;
 
-	mt = kzalloc(struct_size(mt, slots, num_slots), GFP_KERNEL);
+	mt = kzalloc(struct_size(mt, slots, num_slots), GFP_KERNEL | __GFP_NOWARN);
 	if (!mt)
 		goto err_mem;
 
@@ -80,8 +80,8 @@ int input_mt_init_slots(struct input_dev *dev, unsigned int num_slots,
 	if (flags & INPUT_MT_SEMI_MT)
 		__set_bit(INPUT_PROP_SEMI_MT, dev->propbit);
 	if (flags & INPUT_MT_TRACK) {
-		unsigned int n2 = num_slots * num_slots;
-		mt->red = kcalloc(n2, sizeof(*mt->red), GFP_KERNEL);
+		mt->red = kcalloc(array_size(num_slots, num_slots),
+				  sizeof(*mt->red), GFP_KERNEL | __GFP_NOWARN);
 		if (!mt->red)
 			goto err_mem;
 	}
-- 
2.34.1

Re: [PATCH] Input: MT - use __GFP_NOWARN allocation at input_mt_init_slots()

[Dmitry Torokhov]

Hi Tetsuo,

On Sat, Nov 19, 2022 at 04:09:56PM +0900, Tetsuo Handa wrote:
> syzbot is reporting too large allocation at input_mt_init_slots() {1], for
> num_slots is supplied from userspace using ioctl(UI_DEV_CREATE).
> Also, replace n2 with array_size(), for 32bits variable n2 will overflow
> if num_slots >= 65536.

Not really keen on fiddling with the memory allocator flags just to
appease syzbot. Maybe keep them as is, and simply limit the number of
slots to something more reasonable, like 64, and return -EINVAL if it is
above?

> 
> Link: https://syzkaller.appspot.com/bug?extid=0122fa359a69694395d5 [1]
> Reported-by: syzbot <syzbot+0122fa359a69694395d5@syzkaller.appspotmail.com>
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> ---
>  drivers/input/input-mt.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/input/input-mt.c b/drivers/input/input-mt.c
> index 14b53dac1253..cf74579462ba 100644
> --- a/drivers/input/input-mt.c
> +++ b/drivers/input/input-mt.c
> @@ -47,7 +47,7 @@ int input_mt_init_slots(struct input_dev *dev, unsigned int num_slots,
>  	if (mt)
>  		return mt->num_slots != num_slots ? -EINVAL : 0;
>  
> -	mt = kzalloc(struct_size(mt, slots, num_slots), GFP_KERNEL);
> +	mt = kzalloc(struct_size(mt, slots, num_slots), GFP_KERNEL | __GFP_NOWARN);
>  	if (!mt)
>  		goto err_mem;
>  
> @@ -80,8 +80,8 @@ int input_mt_init_slots(struct input_dev *dev, unsigned int num_slots,
>  	if (flags & INPUT_MT_SEMI_MT)
>  		__set_bit(INPUT_PROP_SEMI_MT, dev->propbit);
>  	if (flags & INPUT_MT_TRACK) {
> -		unsigned int n2 = num_slots * num_slots;
> -		mt->red = kcalloc(n2, sizeof(*mt->red), GFP_KERNEL);
> +		mt->red = kcalloc(array_size(num_slots, num_slots),
> +				  sizeof(*mt->red), GFP_KERNEL | __GFP_NOWARN);
>  		if (!mt->red)
>  			goto err_mem;
>  	}
> -- 
> 2.34.1
> 
> 

Thanks.

-- 
Dmitry

Re: [PATCH] Input: MT - use __GFP_NOWARN allocation at input_mt_init_slots()

[Tetsuo Handa]

On 2022/11/23 8:23, Dmitry Torokhov wrote:
> Hi Tetsuo,
> 
> On Sat, Nov 19, 2022 at 04:09:56PM +0900, Tetsuo Handa wrote:
>> syzbot is reporting too large allocation at input_mt_init_slots() {1], for
>> num_slots is supplied from userspace using ioctl(UI_DEV_CREATE).
>> Also, replace n2 with array_size(), for 32bits variable n2 will overflow
>> if num_slots >= 65536.
> 
> Not really keen on fiddling with the memory allocator flags just to
> appease syzbot. Maybe keep them as is, and simply limit the number of
> slots to something more reasonable, like 64, and return -EINVAL if it is
> above?
> 

Hmm, although most users seem to pass values within "unsigned char" range,
not limited to syzbot.

drivers/input/misc/uinput.c has

	nslot = input_abs_get_max(dev, ABS_MT_SLOT) + 1;
	error = input_mt_init_slots(dev, nslot, 0);

and drivers/virtio/virtio_input.c has

	nslots = input_abs_get_max(vi->idev, ABS_MT_SLOT) + 1;
	err = input_mt_init_slots(vi->idev, nslots, 0);

and drivers/input/misc/xen-kbdfront.c has

	int num_cont, width, height;
	num_cont = xenbus_read_unsigned(info->xbdev->otherend,
					XENKBD_FIELD_MT_NUM_CONTACTS,
					1);
	ret = input_mt_init_slots(mtouch, num_cont, INPUT_MT_DIRECT);

. Since these users might need to pass values beyond "unsigned char" range,
I think we should not limit to too small values like 64.

More worrisome thing is that several users are not handling
input_mt_init_slots() failures.

[PATCH v2] 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>
---
Changes in v2:
  Limit max slots instead of using __GFP_NOWARN.

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

Re: [PATCH v2] Input: MT - limit max slots

[Tetsuo Handa]

Who can take this patch?

On 2024/04/27 20:15, 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>
> ---
> Changes in v2:
>   Limit max slots instead of using __GFP_NOWARN.
> 
>  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)