| Message ID | 6d878e01-6c2f-8766-2578-c95030442369@I-love.SAKURA.ne.jp (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Input: MT - use __GFP_NOWARN allocation at input_mt_init_slots() | expand |
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.
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.
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; }
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(-)