diff mbox series

[riscv64] kobject: fix WARNING in input_register_device

Message ID tencent_AF9E941B3D4BEF1B2625D4BA18BBDA332108@qq.com
State New
Headers
Series [riscv64] kobject: fix WARNING in input_register_device |

Commit Message

Edward Adam Davis Feb. 8, 2024, 10:46 a.m. UTC 
  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+8e41bb0c055b209ebbf4@syzkaller.appspotmail.com
Signed-off-by: Edward Adam Davis <eadavis@qq.com>
---
 include/linux/kobject.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Greg KH Feb. 8, 2024, 10:56 a.m. UTC | #1 
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+8e41bb0c055b209ebbf4@syzkaller.appspotmail.com
> Signed-off-by: Edward Adam Davis <eadavis@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 Feb. 8, 2024, 11:37 a.m. UTC | #2 
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+8e41bb0c055b209ebbf4@syzkaller.appspotmail.com
> > Signed-off-by: Edward Adam Davis <eadavis@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 Feb. 8, 2024, 12:25 p.m. UTC | #3 
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+8e41bb0c055b209ebbf4@syzkaller.appspotmail.com
> > > Signed-off-by: Edward Adam Davis <eadavis@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 Feb. 8, 2024, 12:25 p.m. UTC | #4 
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 Feb. 13, 2024, 12:43 a.m. UTC | #5 
On Thu, 8 Feb 2024 12:25:10 +0000, Greg KH wrote:
> 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+8e41bb0c055b209ebbf4@syzkaller.appspotmail.com
> > > > Signed-off-by: Edward Adam Davis <eadavis@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?
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 Feb. 13, 2024, 12:50 a.m. UTC | #6 
On Thu, 8 Feb 2024 12:25:35 +0000, Greg KH wrote:
> 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?
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 Feb. 13, 2024, 7:20 a.m. UTC | #7 
On Tue, Feb 13, 2024 at 08:43:26AM +0800, Edward Adam Davis wrote:
> On Thu, 8 Feb 2024 12:25:10 +0000, Greg KH wrote:
> > 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+8e41bb0c055b209ebbf4@syzkaller.appspotmail.com
> > > > > Signed-off-by: Edward Adam Davis <eadavis@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?
> 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.

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 Feb. 13, 2024, 7:21 a.m. UTC | #8 
On Tue, Feb 13, 2024 at 08:50:01AM +0800, Edward Adam Davis wrote:
> On Thu, 8 Feb 2024 12:25:35 +0000, Greg KH wrote:
> > 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?
> Because when syzbot reported this issue, it wrote "userspace arch: riscv64".
> However, I actually tested it on the master branch of upstream.

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

thanks,

greg k-h
Edward Adam Davis Feb. 13, 2024, 1:10 p.m. UTC | #9 
On Tue, 13 Feb 2024 08:20:52 +0100, Greg KH wrote:
> On Thu, 8 Feb 2024 10:56:00, Greg KH wrote:
> On Tue, Feb 13, 2024 at 08:43:26AM +0800, Edward Adam Davis wrote:
> > On Thu, 8 Feb 2024 12:25:10 +0000, Greg KH wrote:
> > > 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+8e41bb0c055b209ebbf4@syzkaller.appspotmail.com
> > > > > > Signed-off-by: Edward Adam Davis <eadavis@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?
> > 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.
> 
> So your change above is wrong and will not work for the max size?
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.
> 
> > 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? :)
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 Feb. 13, 2024, 1:13 p.m. UTC | #10 
On Tue, 13 Feb 2024 08:21:18 +0100, Greg KH wrote:
> On Tue, Feb 13, 2024 at 08:50:01AM +0800, Edward Adam Davis wrote:
> > On Thu, 8 Feb 2024 12:25:35 +0000, Greg KH wrote:
> > > 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?
> > Because when syzbot reported this issue, it wrote "userspace arch: riscv64".
> > However, I actually tested it on the master branch of upstream.
> 
> Then of course, this was not correct in the subject line.
Got it.

thanks,
edward.
diff mbox series

Patch

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 */