[syzbot] [input?] [usb?] WARNING in input_register_device (2)

[syzbot]

Hello,

syzbot found the following issue on:

HEAD commit: 6613476e225e Linux 6.8-rc1
git tree: git://git.kernel.org/pub/scm/linux/kernel/git/riscv/linux.git fixes
console output: https://syzkaller.appspot.com/x/log.txt?x=15ca9224180000
kernel config: https://syzkaller.appspot.com/x/.config?x=877e61347079aad5
dashboard link: https://syzkaller.appspot.com/bug?extid=8e41bb0c055b209ebbf4
compiler: riscv64-linux-gnu-gcc (Debian 12.2.0-13) 12.2.0, GNU ld (GNU Binutils for Debian) 2.40
userspace arch: riscv64
syz repro: https://syzkaller.appspot.com/x/repro.syz?x=10b32118180000
C reproducer: https://syzkaller.appspot.com/x/repro.c?x=17dab048180000

Downloadable assets:
disk image (non-bootable): https://storage.googleapis.com/syzbot-assets/a741b348759c/non_bootable_disk-6613476e.raw.xz
vmlinux: https://storage.googleapis.com/syzbot-assets/33ea806d02dd/vmlinux-6613476e.xz
kernel image: https://storage.googleapis.com/syzbot-assets/33195f72f823/Image-6613476e.xz

IMPORTANT: if you fix the issue, please add the following tag to the commit:
Reported-by: syzbot+8e41bb...@syzkaller.appspotmail.com

WARNING: CPU: 0 PID: 8 at lib/kobject_uevent.c:671 add_uevent_var+0x282/0x296 lib/kobject_uevent.c:671
Modules linked in:
CPU: 0 PID: 8 Comm: kworker/0:0 Not tainted 6.8.0-rc1-syzkaller #0
Hardware name: riscv-virtio,qemu (DT)
Workqueue: usb_hub_wq hub_event
epc : add_uevent_var+0x282/0x296 lib/kobject_uevent.c:671
ra : add_uevent_var+0x282/0x296 lib/kobject_uevent.c:671
epc : ffffffff858325ec ra : ffffffff858325ec sp : ff20000000049b20
gp : ffffffff8863e320 tp : ff6000000c29ce00 t0 : ccb0ea972aeb292d
t1 : ffe3ffff000092d8 t2 : ff6000000c29d8f8 s0 : ff20000000049c00
s1 : ff60000016d76000 a0 : 0000000000000001 a1 : 0000000000000000
a2 : 0000000000000002 a3 : ffffffff800b66fa a4 : 0000000000000000
a5 : 0000000000000000 a6 : 0000000000000003 a7 : ff200000000496c7
s2 : 0000000000000004 s3 : 1fe4000000009368 s4 : 1fec000002daec43
s5 : ff60000016d76218 s6 : 0000000000000006 s7 : 00000000000007fc
s8 : ff60000016d76a1c s9 : 1fec000002daed43 s10: ffffffff885377c0
s11: ffffffff86b9c560 t3 : 0000000000000004 t4 : ffe3ffff000092d8
t5 : ffe3ffff000092d9 t6 : ff200000000496d8
status: 0000000200000120 badaddr: 0000000000000000 cause: 0000000000000003
[<ffffffff858325ec>] add_uevent_var+0x282/0x296 lib/kobject_uevent.c:671
[<ffffffff85833cdc>] kobject_uevent_env+0xb0c/0x1378 lib/kobject_uevent.c:602
[<ffffffff8583456a>] kobject_uevent+0x22/0x2e lib/kobject_uevent.c:642
[<ffffffff82542b12>] device_add+0x10d2/0x186e drivers/base/core.c:3606
[<ffffffff837032c4>] input_register_device+0x606/0xe9a drivers/input/input.c:2379
[<ffffffff83faba00>] hidinput_connect+0x4c72/0x88a4 drivers/hid/hid-input.c:2355
[<ffffffff83f9d330>] hid_connect+0x11e0/0x15e0 drivers/hid/hid-core.c:2194
[<ffffffff83f9d7e6>] hid_hw_start drivers/hid/hid-core.c:2309 [inline]
[<ffffffff83f9d7e6>] hid_hw_start+0xb6/0x13c drivers/hid/hid-core.c:2300
[<ffffffff840255a0>] ms_probe+0x15e/0x4f0 drivers/hid/hid-microsoft.c:391
[<ffffffff83f9de94>] __hid_device_probe drivers/hid/hid-core.c:2633 [inline]
[<ffffffff83f9de94>] hid_device_probe+0x2a4/0x3f2 drivers/hid/hid-core.c:2670
[<ffffffff8254cd2e>] call_driver_probe drivers/base/dd.c:579 [inline]
[<ffffffff8254cd2e>] really_probe+0x234/0xbbc drivers/base/dd.c:658
[<ffffffff8254d88a>] __driver_probe_device+0x1d4/0x458 drivers/base/dd.c:800
[<ffffffff8254db6e>] driver_probe_device+0x60/0x1ce drivers/base/dd.c:830
[<ffffffff8254dec0>] __device_attach_driver+0x1e4/0x2fe drivers/base/dd.c:958
[<ffffffff82547562>] bus_for_each_drv+0x142/0x1da drivers/base/bus.c:457
[<ffffffff8254eae2>] __device_attach+0x1c4/0x462 drivers/base/dd.c:1030
[<ffffffff8254f108>] device_initial_probe+0x1c/0x26 drivers/base/dd.c:1079
[<ffffffff82549fe4>] bus_probe_device+0x15c/0x192 drivers/base/bus.c:532
[<ffffffff82542b6c>] device_add+0x112c/0x186e drivers/base/core.c:3625
[<ffffffff83f96c5c>] hid_add_device+0x374/0x9b2 drivers/hid/hid-core.c:2816
[<ffffffff840dc3f2>] usbhid_probe+0xa50/0xf84 drivers/hid/usbhid/hid-core.c:1429
[<ffffffff830a18c0>] usb_probe_interface+0x2d4/0x8a2 drivers/usb/core/driver.c:399
[<ffffffff8254cd2e>] call_driver_probe drivers/base/dd.c:579 [inline]
[<ffffffff8254cd2e>] really_probe+0x234/0xbbc drivers/base/dd.c:658
[<ffffffff8254d88a>] __driver_probe_device+0x1d4/0x458 drivers/base/dd.c:800
[<ffffffff8254db6e>] driver_probe_device+0x60/0x1ce drivers/base/dd.c:830
[<ffffffff8254dec0>] __device_attach_driver+0x1e4/0x2fe drivers/base/dd.c:958
[<ffffffff82547562>] bus_for_each_drv+0x142/0x1da drivers/base/bus.c:457
[<ffffffff8254eae2>] __device_attach+0x1c4/0x462 drivers/base/dd.c:1030
[<ffffffff8254f108>] device_initial_probe+0x1c/0x26 drivers/base/dd.c:1079
[<ffffffff82549fe4>] bus_probe_device+0x15c/0x192 drivers/base/bus.c:532
[<ffffffff82542b6c>] device_add+0x112c/0x186e drivers/base/core.c:3625
[<ffffffff8309b2b4>] usb_set_configuration+0xfe0/0x1b10 drivers/usb/core/message.c:2207
[<ffffffff830c3d56>] usb_generic_driver_probe+0xae/0x128 drivers/usb/core/generic.c:254
[<ffffffff830a0606>] usb_probe_device+0xd6/0x340 drivers/usb/core/driver.c:294
[<ffffffff8254cd2e>] call_driver_probe drivers/base/dd.c:579 [inline]
[<ffffffff8254cd2e>] really_probe+0x234/0xbbc drivers/base/dd.c:658
[<ffffffff8254d88a>] __driver_probe_device+0x1d4/0x458 drivers/base/dd.c:800
[<ffffffff8254db6e>] driver_probe_device+0x60/0x1ce drivers/base/dd.c:830
[<ffffffff8254dec0>] __device_attach_driver+0x1e4/0x2fe drivers/base/dd.c:958
[<ffffffff82547562>] bus_for_each_drv+0x142/0x1da drivers/base/bus.c:457
[<ffffffff8254eae2>] __device_attach+0x1c4/0x462 drivers/base/dd.c:1030
[<ffffffff8254f108>] device_initial_probe+0x1c/0x26 drivers/base/dd.c:1079
[<ffffffff82549fe4>] bus_probe_device+0x15c/0x192 drivers/base/bus.c:532
[<ffffffff82542b6c>] device_add+0x112c/0x186e drivers/base/core.c:3625
[<ffffffff830777fe>] usb_new_device+0x960/0x1648 drivers/usb/core/hub.c:2596
[<ffffffff8307dc48>] hub_port_connect drivers/usb/core/hub.c:5465 [inline]
[<ffffffff8307dc48>] hub_port_connect_change drivers/usb/core/hub.c:5605 [inline]
[<ffffffff8307dc48>] port_event drivers/usb/core/hub.c:5765 [inline]
[<ffffffff8307dc48>] hub_event+0x2954/0x4756 drivers/usb/core/hub.c:5847
[<ffffffff801231ea>] process_one_work+0x7ce/0x179c kernel/workqueue.c:2633
[<ffffffff80124c94>] process_scheduled_works kernel/workqueue.c:2706 [inline]
[<ffffffff80124c94>] worker_thread+0xadc/0x10f8 kernel/workqueue.c:2787
[<ffffffff80144154>] kthread+0x28c/0x3a6 kernel/kthread.c:388
[<ffffffff85928722>] ret_from_fork+0xe/0x1c arch/riscv/kernel/entry.S:229


---
This report is generated by a bot. It may contain errors.
See https://goo.gl/tpsmEJ for more information about syzbot.
syzbot engineers can be reached at syzk...@googlegroups.com.

syzbot will keep track of this issue. See:
https://goo.gl/tpsmEJ#status for how to communicate with syzbot.

If the report is already addressed, let syzbot know by replying with:
#syz fix: exact-commit-title

If you want syzbot to run the reproducer, reply with:
#syz test: git://repo/address.git branch-or-commit-hash
If you attach or paste a git patch, syzbot will apply it before testing.

If you want to overwrite report's subsystems, reply with:
#syz set subsystems: new-subsystem
(See the list of subsystem names on the web dashboard)

If the report is a duplicate of another one, reply with:
#syz dup: exact-subject-of-another-report

If you want to undo deduplication, reply with:
#syz undup

[Edward Adam Davis]

please test oob

#syz test https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master

diff --git a/include/linux/kobject.h b/include/linux/kobject.h
index c30affcc43b4..74b37b6459cd 100644
--- a/include/linux/kobject.h
+++ b/include/linux/kobject.h
@@ -30,7 +30,7 @@

#define UEVENT_HELPER_PATH_LEN 256
#define UEVENT_NUM_ENVP 64 /* number of env pointers */
-#define UEVENT_BUFFER_SIZE 2048 /* buffer for the variables */
+#define UEVENT_BUFFER_SIZE 2560 /* buffer for the variables */

#ifdef CONFIG_UEVENT_HELPER
/* path to the userspace helper executed on an event */

[syzbot]

Hello,

syzbot has tested the proposed patch and the reproducer did not trigger any issue:

Reported-and-tested-by: syzbot+8e41bb...@syzkaller.appspotmail.com

Tested on:

commit: 04737196 Merge tag 'v6.8-p3' of git://git.kernel.org/p..
git tree: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
console output: https://syzkaller.appspot.com/x/log.txt?x=13ab9118180000
kernel config: https://syzkaller.appspot.com/x/.config?x=a569be813824c2a8

dashboard link: https://syzkaller.appspot.com/bug?extid=8e41bb0c055b209ebbf4
compiler: riscv64-linux-gnu-gcc (Debian 12.2.0-13) 12.2.0, GNU ld (GNU Binutils for Debian) 2.40
userspace arch: riscv64

patch: https://syzkaller.appspot.com/x/patch.diff?x=12fc85d4180000

Note: testing is done by a robot and is best-effort only.

[Edward Adam Davis]

The input_add_uevent_modalias_var()->input_print_modalias() will add 1684 bytes
of data to env, which will result in insufficient memory allocated to the buf
members of env.

Reported-and-tested-by: syzbot+8e41bb...@syzkaller.appspotmail.com
Signed-off-by: Edward Adam Davis <ead...@qq.com>
---
include/linux/kobject.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/kobject.h b/include/linux/kobject.h
index c30affcc43b4..74b37b6459cd 100644
--- a/include/linux/kobject.h
+++ b/include/linux/kobject.h
@@ -30,7 +30,7 @@

#define UEVENT_HELPER_PATH_LEN 256
#define UEVENT_NUM_ENVP 64 /* number of env pointers */
-#define UEVENT_BUFFER_SIZE 2048 /* buffer for the variables */
+#define UEVENT_BUFFER_SIZE 2560 /* buffer for the variables */

#ifdef CONFIG_UEVENT_HELPER
/* path to the userspace helper executed on an event */

--
2.43.0

[Greg KH]

On Thu, Feb 08, 2024 at 06:46:55PM +0800, Edward Adam Davis wrote:
> The input_add_uevent_modalias_var()->input_print_modalias() will add 1684 bytes
> of data to env, which will result in insufficient memory allocated to the buf
> members of env.

What is "env"? And can you wrap your lines at 72 columns please?

> Reported-and-tested-by: syzbot+8e41bb...@syzkaller.appspotmail.com
> Signed-off-by: Edward Adam Davis <ead...@qq.com>
> ---
> include/linux/kobject.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/linux/kobject.h b/include/linux/kobject.h
> index c30affcc43b4..74b37b6459cd 100644
> --- a/include/linux/kobject.h
> +++ b/include/linux/kobject.h
> @@ -30,7 +30,7 @@
>
> #define UEVENT_HELPER_PATH_LEN 256
> #define UEVENT_NUM_ENVP 64 /* number of env pointers */
> -#define UEVENT_BUFFER_SIZE 2048 /* buffer for the variables */
> +#define UEVENT_BUFFER_SIZE 2560 /* buffer for the variables */

That's an odd number, why that? Why not just a page? What happens if
some other path wants more?

And what's causing the input stack to have so many variables all of a
sudden, what changed to cause this? Is this a bugfix for a specific
commit that needs to be backported to older kernels? Why did this
buffer size all of a sudden be too small?

thanks,

greg k-h

[Edward Adam Davis]

On Thu, 8 Feb 2024 10:56:00, Greg KH wrote:
> > The input_add_uevent_modalias_var()->input_print_modalias() will add 1684 bytes
> > of data to env, which will result in insufficient memory allocated to the buf
> > members of env.
>
> What is "env"? And can you wrap your lines at 72 columns please?

env is an instance of struct kobj_uevent_env.

>
> > Reported-and-tested-by: syzbot+8e41bb...@syzkaller.appspotmail.com
> > Signed-off-by: Edward Adam Davis <ead...@qq.com>
> > ---
> > include/linux/kobject.h | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/include/linux/kobject.h b/include/linux/kobject.h
> > index c30affcc43b4..74b37b6459cd 100644
> > --- a/include/linux/kobject.h
> > +++ b/include/linux/kobject.h
> > @@ -30,7 +30,7 @@
> >
> > #define UEVENT_HELPER_PATH_LEN 256
> > #define UEVENT_NUM_ENVP 64 /* number of env pointers */
> > -#define UEVENT_BUFFER_SIZE 2048 /* buffer for the variables */
> > +#define UEVENT_BUFFER_SIZE 2560 /* buffer for the variables */
>
> That's an odd number, why that? Why not just a page? What happens if
> some other path wants more?

An increase of 512 bytes is sufficient for the current issue. Do not consider
the problem of hypothetical existence.

>
> And what's causing the input stack to have so many variables all of a
> sudden, what changed to cause this? Is this a bugfix for a specific
> commit that needs to be backported to older kernels? Why did this
> buffer size all of a sudden be too small?

The result of my analysis is that several members of struct input_dev are too
large, such as its member keybit.

thanks,
edward.

[Greg KH]

On Thu, Feb 08, 2024 at 07:37:56PM +0800, Edward Adam Davis wrote:
> On Thu, 8 Feb 2024 10:56:00, Greg KH wrote:
> > > The input_add_uevent_modalias_var()->input_print_modalias() will add 1684 bytes
> > > of data to env, which will result in insufficient memory allocated to the buf
> > > members of env.
> >
> > What is "env"? And can you wrap your lines at 72 columns please?
> env is an instance of struct kobj_uevent_env.

Ok, be specific please in your changelog text, otherwise we can't really
understand what is happening.

> > > Reported-and-tested-by: syzbot+8e41bb...@syzkaller.appspotmail.com
> > > Signed-off-by: Edward Adam Davis <ead...@qq.com>
> > > ---
> > > include/linux/kobject.h | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/include/linux/kobject.h b/include/linux/kobject.h
> > > index c30affcc43b4..74b37b6459cd 100644
> > > --- a/include/linux/kobject.h
> > > +++ b/include/linux/kobject.h
> > > @@ -30,7 +30,7 @@
> > >
> > > #define UEVENT_HELPER_PATH_LEN 256
> > > #define UEVENT_NUM_ENVP 64 /* number of env pointers */
> > > -#define UEVENT_BUFFER_SIZE 2048 /* buffer for the variables */
> > > +#define UEVENT_BUFFER_SIZE 2560 /* buffer for the variables */
> >
> > That's an odd number, why that? Why not just a page? What happens if
> > some other path wants more?
> An increase of 512 bytes is sufficient for the current issue. Do not consider
> the problem of hypothetical existence.

Why is this 512 bytes sufficient now? What changed to cause this?

And how can we detect this automatically in the future? Shouldn't we
just be truncating the buffer instead of having an overflow?

> > And what's causing the input stack to have so many variables all of a
> > sudden, what changed to cause this? Is this a bugfix for a specific
> > commit that needs to be backported to older kernels? Why did this
> > buffer size all of a sudden be too small?
> The result of my analysis is that several members of struct input_dev are too
> large, such as its member keybit.

And when did that change? What commit id? What prevents it from
growing again and us needing to change this again?

thanks,

greg k-h

[Greg KH]

On Thu, Feb 08, 2024 at 07:37:56PM +0800, Edward Adam Davis wrote:
> On Thu, 8 Feb 2024 10:56:00, Greg KH wrote:
> > > The input_add_uevent_modalias_var()->input_print_modalias() will add 1684 bytes
> > > of data to env, which will result in insufficient memory allocated to the buf
> > > members of env.
> >
> > What is "env"? And can you wrap your lines at 72 columns please?
> env is an instance of struct kobj_uevent_env.

Also, why is "risc64" in the subject line?

confused,

greg k-h

[Edward Adam Davis]

There is the following code in input_print_modalias():

drivers/input/input.c
1 len += input_print_modalias_bits(buf + len, size - len,
1403 'k', id->keybit, KEY_MIN_INTERESTING, KEY_MAX);
This code will add up to 2608 bytes of data to env at most.
(KEY_MAX - KEY_MIN_INTERESTING) * 4 = (256 * 3 - 1 - 113 ) * 4 = (765 - 113) * 4 = 652 * 4 = 2608 bytes。
Note: In the expression, 4 represents 3 bytes of hexadecimal data and 1 byte of comma.

include/uapi/linux/input-event-codes.h
188 #define KEY_MUTE 113
807 #define KEY_MIN_INTERESTING KEY_MUTE
808 #define KEY_MAX 0x2ff
During my actual testing process, I found that a total of 1684 bytes were
contributed in input_print_modalias().

>
> And how can we detect this automatically in the future? Shouldn't we
> just be truncating the buffer instead of having an overflow?
>
> > > And what's causing the input stack to have so many variables all of a
> > > sudden, what changed to cause this? Is this a bugfix for a specific
> > > commit that needs to be backported to older kernels? Why did this
> > > buffer size all of a sudden be too small?
> > The result of my analysis is that several members of struct input_dev are too
> > large, such as its member keybit.
>
> And when did that change? What commit id? What prevents it from
> growing again and us needing to change this again?

The code that caused this issue has been introduced for a long time, and it is
speculated that it was due to the fact that the warning in add_uevent_var() was
returned directly to ENOMEM without being taken seriously.

lib/kobject_uevent.c
2 if (len >= (sizeof(env->buf) - env->buflen)) {
1 WARN(1, KERN_ERR "add_uevent_var: buffer size too small\n");
672 return -ENOMEM;
1 }

I believe that this issue was introduced by:
7eff2e7a8b65 - Driver core: change add_ueventvar to use a struct.

thanks,
edward.

[Edward Adam Davis]

Because when syzbot reported this issue, it wrote "userspace arch: riscv64".
However, I actually tested it on the master branch of upstream.

thanks,
edward.

[Greg KH]

So your change above is wrong and will not work for the max size?

Why not restrict the modalias here to fit instead of overflowing? Odds
are we should be checking this properly no matter what the value is
changed to, right?

> include/uapi/linux/input-event-codes.h
> 188 #define KEY_MUTE 113
> 807 #define KEY_MIN_INTERESTING KEY_MUTE
> 808 #define KEY_MAX 0x2ff
> During my actual testing process, I found that a total of 1684 bytes were
> contributed in input_print_modalias().
> >
> > And how can we detect this automatically in the future? Shouldn't we
> > just be truncating the buffer instead of having an overflow?
> >
> > > > And what's causing the input stack to have so many variables all of a
> > > > sudden, what changed to cause this? Is this a bugfix for a specific
> > > > commit that needs to be backported to older kernels? Why did this
> > > > buffer size all of a sudden be too small?
> > > The result of my analysis is that several members of struct input_dev are too
> > > large, such as its member keybit.
> >
> > And when did that change? What commit id? What prevents it from
> > growing again and us needing to change this again?
> The code that caused this issue has been introduced for a long time, and it is
> speculated that it was due to the fact that the warning in add_uevent_var() was
> returned directly to ENOMEM without being taken seriously.
>
> lib/kobject_uevent.c
> 2 if (len >= (sizeof(env->buf) - env->buflen)) {
> 1 WARN(1, KERN_ERR "add_uevent_var: buffer size too small\n");
> 672 return -ENOMEM;

Odd line numbers?

Anyway, we should get rid of the WARN() as that will cause crashes, and
just handle it properly there.

> 1 }
>
> I believe that this issue was introduced by:
> 7eff2e7a8b65 - Driver core: change add_ueventvar to use a struct.

In 2007? And never been actually hit since then? So is this a real
issue? :)

thanks,

greg k-h

[Greg KH]

Then of course, this was not correct in the subject line.

thanks,

greg k-h

[Edward Adam Davis]

Yes.

>
> Why not restrict the modalias here to fit instead of overflowing? Odds
> are we should be checking this properly no matter what the value is
> changed to, right?

Right.
It may be necessary to deepen our understanding of this piece of code before
fixing this issue internally.

Yes. But as I mentioned earlier, in add_uevent_var(), it will exit directly after
a warning, so this issue has not been given enough attention, perhaps it has
happened many times.

thanks,
edward.

[Edward Adam Davis]

Got it.

thanks,
edward.