1. Patchew
  2. linux
  3. View series

[PATCH v2] kobject_uevent: Fix OOB access within zap_modalias_env()

Zijun Hu posted 1 patch 1 year, 8 months ago
There is a newer version of this series
lib/kobject_uevent.c | 17 ++++++++++++++++-
1 file changed, 16 insertions(+), 1 deletion(-)
[PATCH v2] kobject_uevent: Fix OOB access within zap_modalias_env()
Posted by Zijun Hu 1 year, 8 months ago 
zap_modalias_env() wrongly calculates size of memory block to move, so
will cause OOB memory access issue if variable MODALIAS is not the last
one within its @env parameter, fixed by correcting size to memmove.

Fixes: 9b3fa47d4a76 ("kobject: fix suppressing modalias in uevents delivered over netlink")
Cc: stable@vger.kernel.org
Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
---
V1 -> V2: Correct commit messages and add inline comments

V1 discussion link:
https://lore.kernel.org/lkml/0b916393-eb39-4467-9c99-ac1bc9746512@quicinc.com/T/#m8d80165294640dbac72f5c48d14b7ca4f097b5c7

 lib/kobject_uevent.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/lib/kobject_uevent.c b/lib/kobject_uevent.c
index 03b427e2707e..f22366be020c 100644
--- a/lib/kobject_uevent.c
+++ b/lib/kobject_uevent.c
@@ -433,8 +433,23 @@ static void zap_modalias_env(struct kobj_uevent_env *env)
 		len = strlen(env->envp[i]) + 1;
 
 		if (i != env->envp_idx - 1) {
+			/* @env->envp[] contains pointers to @env->buf[]
+			 * with @env->buflen elements, and we want to
+			 * remove variable MODALIAS pointed by
+			 * @env->envp[i] with length @len as shown below:
+			 *
+			 * 0          @env->buf[]      @env->buflen
+			 * ----------------------------------------
+			 *      ^              ^                  ^
+			 *      |->   @len   <-|   target block   |
+			 * @env->envp[i]  @env->envp[i+1]
+			 *
+			 * so the "target block" indicated above is moved
+			 * backward by @len, and its right size is
+			 * (@env->buf + @env->buflen - @env->envp[i + 1]).
+			 */
 			memmove(env->envp[i], env->envp[i + 1],
-				env->buflen - len);
+				env->buf + env->buflen - env->envp[i + 1]);
 
 			for (j = i; j < env->envp_idx - 1; j++)
 				env->envp[j] = env->envp[j + 1] - len;
-- 
2.7.4
Re: [PATCH v2] kobject_uevent: Fix OOB access within zap_modalias_env()
Posted by Dmitry Torokhov 1 year, 8 months ago 
On Tue, May 28, 2024 at 11:19:07AM +0800, Zijun Hu wrote:
> zap_modalias_env() wrongly calculates size of memory block to move, so
> will cause OOB memory access issue if variable MODALIAS is not the last
> one within its @env parameter, fixed by correcting size to memmove.
> 
> Fixes: 9b3fa47d4a76 ("kobject: fix suppressing modalias in uevents delivered over netlink")
> Cc: stable@vger.kernel.org
> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
> ---
> V1 -> V2: Correct commit messages and add inline comments
> 
> V1 discussion link:
> https://lore.kernel.org/lkml/0b916393-eb39-4467-9c99-ac1bc9746512@quicinc.com/T/#m8d80165294640dbac72f5c48d14b7ca4f097b5c7
> 
>  lib/kobject_uevent.c | 17 ++++++++++++++++-
>  1 file changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/kobject_uevent.c b/lib/kobject_uevent.c
> index 03b427e2707e..f22366be020c 100644
> --- a/lib/kobject_uevent.c
> +++ b/lib/kobject_uevent.c
> @@ -433,8 +433,23 @@ static void zap_modalias_env(struct kobj_uevent_env *env)
>  		len = strlen(env->envp[i]) + 1;
>  
>  		if (i != env->envp_idx - 1) {
> +			/* @env->envp[] contains pointers to @env->buf[]
> +			 * with @env->buflen elements, and we want to
> +			 * remove variable MODALIAS pointed by
> +			 * @env->envp[i] with length @len as shown below:
> +			 *
> +			 * 0          @env->buf[]      @env->buflen
> +			 * ----------------------------------------
> +			 *      ^              ^                  ^
> +			 *      |->   @len   <-|   target block   |
> +			 * @env->envp[i]  @env->envp[i+1]
> +			 *
> +			 * so the "target block" indicated above is moved
> +			 * backward by @len, and its right size is
> +			 * (@env->buf + @env->buflen - @env->envp[i + 1]).
> +			 */
>  			memmove(env->envp[i], env->envp[i + 1],
> -				env->buflen - len);
> +				env->buf + env->buflen - env->envp[i + 1]);

Thank you for noticing this, it is indeed a bug.

I wonder if this would not be expressed better as:

			tail_len = env->buflen - (env->envp[i + 1] - env->envp[0]);
			memmove(env->envp[i], env->envp[i + 1], tail_len);

and we would not need the large comment.

Otherwise:

Acked-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

>  
>  			for (j = i; j < env->envp_idx - 1; j++)
>  				env->envp[j] = env->envp[j + 1] - len;
> -- 
> 2.7.4
> 

Thanks.

-- 
Dmitry
Re: [PATCH v2] kobject_uevent: Fix OOB access within zap_modalias_env()
Posted by quic_zijuhu 1 year, 8 months ago 
On 5/29/2024 2:56 AM, Dmitry Torokhov wrote:
> On Tue, May 28, 2024 at 11:19:07AM +0800, Zijun Hu wrote:
>> zap_modalias_env() wrongly calculates size of memory block to move, so
>> will cause OOB memory access issue if variable MODALIAS is not the last
>> one within its @env parameter, fixed by correcting size to memmove.
>>
>> Fixes: 9b3fa47d4a76 ("kobject: fix suppressing modalias in uevents delivered over netlink")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
>> ---
>> V1 -> V2: Correct commit messages and add inline comments
>>
>> V1 discussion link:
>> https://lore.kernel.org/lkml/0b916393-eb39-4467-9c99-ac1bc9746512@quicinc.com/T/#m8d80165294640dbac72f5c48d14b7ca4f097b5c7
>>
>>  lib/kobject_uevent.c | 17 ++++++++++++++++-
>>  1 file changed, 16 insertions(+), 1 deletion(-)
>>
>> diff --git a/lib/kobject_uevent.c b/lib/kobject_uevent.c
>> index 03b427e2707e..f22366be020c 100644
>> --- a/lib/kobject_uevent.c
>> +++ b/lib/kobject_uevent.c
>> @@ -433,8 +433,23 @@ static void zap_modalias_env(struct kobj_uevent_env *env)
>>  		len = strlen(env->envp[i]) + 1;
>>  
>>  		if (i != env->envp_idx - 1) {
>> +			/* @env->envp[] contains pointers to @env->buf[]
>> +			 * with @env->buflen elements, and we want to
>> +			 * remove variable MODALIAS pointed by
>> +			 * @env->envp[i] with length @len as shown below:
>> +			 *
>> +			 * 0          @env->buf[]      @env->buflen
>> +			 * ----------------------------------------
>> +			 *      ^              ^                  ^
>> +			 *      |->   @len   <-|   target block   |
>> +			 * @env->envp[i]  @env->envp[i+1]
>> +			 *
>> +			 * so the "target block" indicated above is moved
>> +			 * backward by @len, and its right size is
>> +			 * (@env->buf + @env->buflen - @env->envp[i + 1]).
>> +			 */
>>  			memmove(env->envp[i], env->envp[i + 1],
>> -				env->buflen - len);
>> +				env->buf + env->buflen - env->envp[i + 1]);
> 
> Thank you for noticing this, it is indeed a bug.
> 
> I wonder if this would not be expressed better as:
> 
> 			tail_len = env->buflen - (env->envp[i + 1] - env->envp[0]);
> 			memmove(env->envp[i], env->envp[i + 1], tail_len);
> 
> and we would not need the large comment.
>
Greg KH suggests add inline comments since my fix is not obvious with
first glance, let us wait for his comments within 2 days about below
question:
is it okay to remove those inline comments if block size to move is
changed to env->buflen - (env->envp[i + 1] - env->envp[0]) ?
> Otherwise:
> 
> Acked-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> 
>>  
>>  			for (j = i; j < env->envp_idx - 1; j++)
>>  				env->envp[j] = env->envp[j + 1] - len;
>> -- 
>> 2.7.4
>>
> 
> Thanks.
>
Re: [PATCH v2] kobject_uevent: Fix OOB access within zap_modalias_env()
Posted by Greg KH 1 year, 8 months ago 
On Wed, May 29, 2024 at 06:07:06PM +0800, quic_zijuhu wrote:
> On 5/29/2024 2:56 AM, Dmitry Torokhov wrote:
> > On Tue, May 28, 2024 at 11:19:07AM +0800, Zijun Hu wrote:
> >> zap_modalias_env() wrongly calculates size of memory block to move, so
> >> will cause OOB memory access issue if variable MODALIAS is not the last
> >> one within its @env parameter, fixed by correcting size to memmove.
> >>
> >> Fixes: 9b3fa47d4a76 ("kobject: fix suppressing modalias in uevents delivered over netlink")
> >> Cc: stable@vger.kernel.org
> >> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
> >> ---
> >> V1 -> V2: Correct commit messages and add inline comments
> >>
> >> V1 discussion link:
> >> https://lore.kernel.org/lkml/0b916393-eb39-4467-9c99-ac1bc9746512@quicinc.com/T/#m8d80165294640dbac72f5c48d14b7ca4f097b5c7
> >>
> >>  lib/kobject_uevent.c | 17 ++++++++++++++++-
> >>  1 file changed, 16 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/lib/kobject_uevent.c b/lib/kobject_uevent.c
> >> index 03b427e2707e..f22366be020c 100644
> >> --- a/lib/kobject_uevent.c
> >> +++ b/lib/kobject_uevent.c
> >> @@ -433,8 +433,23 @@ static void zap_modalias_env(struct kobj_uevent_env *env)
> >>  		len = strlen(env->envp[i]) + 1;
> >>  
> >>  		if (i != env->envp_idx - 1) {
> >> +			/* @env->envp[] contains pointers to @env->buf[]
> >> +			 * with @env->buflen elements, and we want to
> >> +			 * remove variable MODALIAS pointed by
> >> +			 * @env->envp[i] with length @len as shown below:
> >> +			 *
> >> +			 * 0          @env->buf[]      @env->buflen
> >> +			 * ----------------------------------------
> >> +			 *      ^              ^                  ^
> >> +			 *      |->   @len   <-|   target block   |
> >> +			 * @env->envp[i]  @env->envp[i+1]
> >> +			 *
> >> +			 * so the "target block" indicated above is moved
> >> +			 * backward by @len, and its right size is
> >> +			 * (@env->buf + @env->buflen - @env->envp[i + 1]).
> >> +			 */
> >>  			memmove(env->envp[i], env->envp[i + 1],
> >> -				env->buflen - len);
> >> +				env->buf + env->buflen - env->envp[i + 1]);
> > 
> > Thank you for noticing this, it is indeed a bug.
> > 
> > I wonder if this would not be expressed better as:
> > 
> > 			tail_len = env->buflen - (env->envp[i + 1] - env->envp[0]);
> > 			memmove(env->envp[i], env->envp[i + 1], tail_len);
> > 
> > and we would not need the large comment.
> >
> Greg KH suggests add inline comments since my fix is not obvious with
> first glance, let us wait for his comments within 2 days about below
> question:
> is it okay to remove those inline comments if block size to move is
> changed to env->buflen - (env->envp[i + 1] - env->envp[0]) ?

I'm all for making this simpler, please do so as Dmitry's response looks
better and easier to understand, don't you think?

And add comments, they are always good here for stuff like this :)

thanks,

greg k-h
Re: [PATCH v2] kobject_uevent: Fix OOB access within zap_modalias_env()
Posted by quic_zijuhu 1 year, 8 months ago 
On 5/29/2024 6:17 PM, Greg KH wrote:
> On Wed, May 29, 2024 at 06:07:06PM +0800, quic_zijuhu wrote:
>> On 5/29/2024 2:56 AM, Dmitry Torokhov wrote:
>>> On Tue, May 28, 2024 at 11:19:07AM +0800, Zijun Hu wrote:
>>>> zap_modalias_env() wrongly calculates size of memory block to move, so
>>>> will cause OOB memory access issue if variable MODALIAS is not the last
>>>> one within its @env parameter, fixed by correcting size to memmove.
>>>>
>>>> Fixes: 9b3fa47d4a76 ("kobject: fix suppressing modalias in uevents delivered over netlink")
>>>> Cc: stable@vger.kernel.org
>>>> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
>>>> ---
>>>> V1 -> V2: Correct commit messages and add inline comments
>>>>
>>>> V1 discussion link:
>>>> https://lore.kernel.org/lkml/0b916393-eb39-4467-9c99-ac1bc9746512@quicinc.com/T/#m8d80165294640dbac72f5c48d14b7ca4f097b5c7
>>>>
>>>>  lib/kobject_uevent.c | 17 ++++++++++++++++-
>>>>  1 file changed, 16 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/lib/kobject_uevent.c b/lib/kobject_uevent.c
>>>> index 03b427e2707e..f22366be020c 100644
>>>> --- a/lib/kobject_uevent.c
>>>> +++ b/lib/kobject_uevent.c
>>>> @@ -433,8 +433,23 @@ static void zap_modalias_env(struct kobj_uevent_env *env)
>>>>  		len = strlen(env->envp[i]) + 1;
>>>>  
>>>>  		if (i != env->envp_idx - 1) {
>>>> +			/* @env->envp[] contains pointers to @env->buf[]
>>>> +			 * with @env->buflen elements, and we want to
>>>> +			 * remove variable MODALIAS pointed by
>>>> +			 * @env->envp[i] with length @len as shown below:
>>>> +			 *
>>>> +			 * 0          @env->buf[]      @env->buflen
>>>> +			 * ----------------------------------------
>>>> +			 *      ^              ^                  ^
>>>> +			 *      |->   @len   <-|   target block   |
>>>> +			 * @env->envp[i]  @env->envp[i+1]
>>>> +			 *
>>>> +			 * so the "target block" indicated above is moved
>>>> +			 * backward by @len, and its right size is
>>>> +			 * (@env->buf + @env->buflen - @env->envp[i + 1]).
>>>> +			 */
>>>>  			memmove(env->envp[i], env->envp[i + 1],
>>>> -				env->buflen - len);
>>>> +				env->buf + env->buflen - env->envp[i + 1]);
>>>
>>> Thank you for noticing this, it is indeed a bug.
>>>
>>> I wonder if this would not be expressed better as:
>>>
>>> 			tail_len = env->buflen - (env->envp[i + 1] - env->envp[0]);
>>> 			memmove(env->envp[i], env->envp[i + 1], tail_len);
>>>
>>> and we would not need the large comment.
>>>
>> Greg KH suggests add inline comments since my fix is not obvious with
>> first glance, let us wait for his comments within 2 days about below
>> question:
>> is it okay to remove those inline comments if block size to move is
>> changed to env->buflen - (env->envp[i + 1] - env->envp[0]) ?
> 
> I'm all for making this simpler, please do so as Dmitry's response looks
> better and easier to understand, don't you think?
> 
> And add comments, they are always good here for stuff like this :)
> 
okay, will send v3 to take Dmitry's suggestion and keep inline comments.
> thanks,
> 
> greg k-h

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.