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

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

[Zijun Hu]

zap_modalias_env() wrongly calculates size of memory block
to move, so maybe cause OOB memory access issue, 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>
---
 lib/kobject_uevent.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/kobject_uevent.c b/lib/kobject_uevent.c
index 03b427e2707e..f153b4f9d4d9 100644
--- a/lib/kobject_uevent.c
+++ b/lib/kobject_uevent.c
@@ -434,7 +434,7 @@ static void zap_modalias_env(struct kobj_uevent_env *env)
 
 		if (i != env->envp_idx - 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] kobject_uevent: Fix OOB access within zap_modalias_env()

[Greg KH]

On Fri, May 24, 2024 at 12:20:03PM +0800, Zijun Hu wrote:
> zap_modalias_env() wrongly calculates size of memory block
> to move, so maybe cause OOB memory access issue, fixed by
> correcting size to memmove.

"maybe" or "does"?  That's a big difference :)

> 
> 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>
> ---
>  lib/kobject_uevent.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/kobject_uevent.c b/lib/kobject_uevent.c
> index 03b427e2707e..f153b4f9d4d9 100644
> --- a/lib/kobject_uevent.c
> +++ b/lib/kobject_uevent.c
> @@ -434,7 +434,7 @@ static void zap_modalias_env(struct kobj_uevent_env *env)
>  
>  		if (i != env->envp_idx - 1) {
>  			memmove(env->envp[i], env->envp[i + 1],
> -				env->buflen - len);
> +				env->buf + env->buflen - env->envp[i + 1]);

How is this "more correct"?  Please explain it better, this logic is not
obvious at all.

thanks,

greg k-h

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

[quic_zijuhu]

On 5/24/2024 12:33 PM, Greg KH wrote:
> On Fri, May 24, 2024 at 12:20:03PM +0800, Zijun Hu wrote:
>> zap_modalias_env() wrongly calculates size of memory block
>> to move, so maybe cause OOB memory access issue, fixed by
>> correcting size to memmove.
> 
> "maybe" or "does"?  That's a big difference :)
> 
i found this issue by reading code instead of really meeting this issue.
this issue should be prone to happen if there are more than 1 other
environment vars.

do you have suggestion about term to use?

>>
>> 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>
>> ---
>>  lib/kobject_uevent.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/lib/kobject_uevent.c b/lib/kobject_uevent.c
>> index 03b427e2707e..f153b4f9d4d9 100644
>> --- a/lib/kobject_uevent.c
>> +++ b/lib/kobject_uevent.c
>> @@ -434,7 +434,7 @@ static void zap_modalias_env(struct kobj_uevent_env *env)
>>  
>>  		if (i != env->envp_idx - 1) {
>>  			memmove(env->envp[i], env->envp[i + 1],
>> -				env->buflen - len);
>> +				env->buf + env->buflen - env->envp[i + 1]);
> 
> How is this "more correct"?  Please explain it better, this logic is not
> obvious at all.
> 
env->envp[] contains pointers to env->buf[] with length env->buflen,
we want to delete environment variable pointed by env->envp[i] with
length @len as shown below.

env->buf[]            |-> target block <-|
0-----------------------------------------env->buflen
        ^             ^
	| ->  @len <- |
  env->envp[i]   env->envp[i+1]

so move "target block" forward by @len, so size of target block is
env->buf + env->buflen - env->envp[i+1] instead of env->buflen
-len.

do you suggest add inline comments to explain it ?

> thanks,
> 
> greg k-h

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

[Greg KH]

On Fri, May 24, 2024 at 01:15:01PM +0800, quic_zijuhu wrote:
> On 5/24/2024 12:33 PM, Greg KH wrote:
> > On Fri, May 24, 2024 at 12:20:03PM +0800, Zijun Hu wrote:
> >> zap_modalias_env() wrongly calculates size of memory block
> >> to move, so maybe cause OOB memory access issue, fixed by
> >> correcting size to memmove.
> > 
> > "maybe" or "does"?  That's a big difference :)
> > 
> i found this issue by reading code instead of really meeting this issue.
> this issue should be prone to happen if there are more than 1 other
> environment vars.

But does it?  Given that we have loads of memory checkers, and I haven't
ever seen any report of any overrun, it would be nice to be sure.

> do you have suggestion about term to use?

Some confirmation that this really is the case would be nice :)

> >> 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>
> >> ---
> >>  lib/kobject_uevent.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/lib/kobject_uevent.c b/lib/kobject_uevent.c
> >> index 03b427e2707e..f153b4f9d4d9 100644
> >> --- a/lib/kobject_uevent.c
> >> +++ b/lib/kobject_uevent.c
> >> @@ -434,7 +434,7 @@ static void zap_modalias_env(struct kobj_uevent_env *env)
> >>  
> >>  		if (i != env->envp_idx - 1) {
> >>  			memmove(env->envp[i], env->envp[i + 1],
> >> -				env->buflen - len);
> >> +				env->buf + env->buflen - env->envp[i + 1]);
> > 
> > How is this "more correct"?  Please explain it better, this logic is not
> > obvious at all.
> > 
> env->envp[] contains pointers to env->buf[] with length env->buflen,
> we want to delete environment variable pointed by env->envp[i] with
> length @len as shown below.
> 
> env->buf[]            |-> target block <-|
> 0-----------------------------------------env->buflen
>         ^             ^
> 	| ->  @len <- |
>   env->envp[i]   env->envp[i+1]
> 
> so move "target block" forward by @len, so size of target block is
> env->buf + env->buflen - env->envp[i+1] instead of env->buflen
> -len.
> 
> do you suggest add inline comments to explain it ?

Yes please.

thanks,

greg k-h

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

[quic_zijuhu]

On 5/24/2024 1:21 PM, Greg KH wrote:
> On Fri, May 24, 2024 at 01:15:01PM +0800, quic_zijuhu wrote:
>> On 5/24/2024 12:33 PM, Greg KH wrote:
>>> On Fri, May 24, 2024 at 12:20:03PM +0800, Zijun Hu wrote:
>>>> zap_modalias_env() wrongly calculates size of memory block
>>>> to move, so maybe cause OOB memory access issue, fixed by
>>>> correcting size to memmove.
>>>
>>> "maybe" or "does"?  That's a big difference :)
>>>
>> i found this issue by reading code instead of really meeting this issue.
>> this issue should be prone to happen if there are more than 1 other
>> environment vars.
> 
> But does it?  Given that we have loads of memory checkers, and I haven't
> ever seen any report of any overrun, it would be nice to be sure.
> 
yes. if @env includes env vairable MODALIAS and  more than one other env
vairables. then (env->buflen - len) must be greater that actual size of
"target block" shown previously, so the OOB issue must happen.
>> do you have suggestion about term to use?
> 
> Some confirmation that this really is the case would be nice :)
> 
>>>> 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>
>>>> ---
>>>>  lib/kobject_uevent.c | 2 +-
>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/lib/kobject_uevent.c b/lib/kobject_uevent.c
>>>> index 03b427e2707e..f153b4f9d4d9 100644
>>>> --- a/lib/kobject_uevent.c
>>>> +++ b/lib/kobject_uevent.c
>>>> @@ -434,7 +434,7 @@ static void zap_modalias_env(struct kobj_uevent_env *env)
>>>>  
>>>>  		if (i != env->envp_idx - 1) {
>>>>  			memmove(env->envp[i], env->envp[i + 1],
>>>> -				env->buflen - len);
>>>> +				env->buf + env->buflen - env->envp[i + 1]);
>>>
>>> How is this "more correct"?  Please explain it better, this logic is not
>>> obvious at all.
>>>
>> env->envp[] contains pointers to env->buf[] with length env->buflen,
>> we want to delete environment variable pointed by env->envp[i] with
>> length @len as shown below.
>>
>> env->buf[]            |-> target block <-|
>> 0-----------------------------------------env->buflen
>>         ^             ^
>> 	| ->  @len <- |
>>   env->envp[i]   env->envp[i+1]
>>
>> so move "target block" forward by @len, so size of target block is
>> env->buf + env->buflen - env->envp[i+1] instead of env->buflen
>> -len.
>>
>> do you suggest add inline comments to explain it ?
> 
> Yes please.
> 
> thanks,
> 
> greg k-h

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

[Greg KH]

On Fri, May 24, 2024 at 01:34:49PM +0800, quic_zijuhu wrote:
> On 5/24/2024 1:21 PM, Greg KH wrote:
> > On Fri, May 24, 2024 at 01:15:01PM +0800, quic_zijuhu wrote:
> >> On 5/24/2024 12:33 PM, Greg KH wrote:
> >>> On Fri, May 24, 2024 at 12:20:03PM +0800, Zijun Hu wrote:
> >>>> zap_modalias_env() wrongly calculates size of memory block
> >>>> to move, so maybe cause OOB memory access issue, fixed by
> >>>> correcting size to memmove.
> >>>
> >>> "maybe" or "does"?  That's a big difference :)
> >>>
> >> i found this issue by reading code instead of really meeting this issue.
> >> this issue should be prone to happen if there are more than 1 other
> >> environment vars.
> > 
> > But does it?  Given that we have loads of memory checkers, and I haven't
> > ever seen any report of any overrun, it would be nice to be sure.
> > 
> yes. if @env includes env vairable MODALIAS and  more than one other env
> vairables. then (env->buflen - len) must be greater that actual size of
> "target block" shown previously, so the OOB issue must happen.

Then why are none of the tools that we have for catching out-of-bound
issues triggered here?  Are the tools broken or is this really just not
ever happening?  It would be good to figure that out...

thanks,

greg k-h

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

[quic_zijuhu]

On 5/24/2024 2:56 PM, Greg KH wrote:
> On Fri, May 24, 2024 at 01:34:49PM +0800, quic_zijuhu wrote:
>> On 5/24/2024 1:21 PM, Greg KH wrote:
>>> On Fri, May 24, 2024 at 01:15:01PM +0800, quic_zijuhu wrote:
>>>> On 5/24/2024 12:33 PM, Greg KH wrote:
>>>>> On Fri, May 24, 2024 at 12:20:03PM +0800, Zijun Hu wrote:
>>>>>> zap_modalias_env() wrongly calculates size of memory block
>>>>>> to move, so maybe cause OOB memory access issue, fixed by
>>>>>> correcting size to memmove.
>>>>>
>>>>> "maybe" or "does"?  That's a big difference :)
>>>>>
>>>> i found this issue by reading code instead of really meeting this issue.
>>>> this issue should be prone to happen if there are more than 1 other
>>>> environment vars.
>>>
>>> But does it?  Given that we have loads of memory checkers, and I haven't
>>> ever seen any report of any overrun, it would be nice to be sure.
>>>
>> yes. if @env includes env vairable MODALIAS and  more than one other env
>> vairables. then (env->buflen - len) must be greater that actual size of
>> "target block" shown previously, so the OOB issue must happen.
> 
> Then why are none of the tools that we have for catching out-of-bound
> issues triggered here?  Are the tools broken or is this really just not
> ever happening?  It would be good to figure that out...
> 
don't know why. perhaps, need to report our case to expert of tools.
> thanks,
> 
> greg k-h

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

[Greg KH]

On Fri, May 24, 2024 at 05:08:06PM +0800, quic_zijuhu wrote:
> On 5/24/2024 2:56 PM, Greg KH wrote:
> > On Fri, May 24, 2024 at 01:34:49PM +0800, quic_zijuhu wrote:
> >> On 5/24/2024 1:21 PM, Greg KH wrote:
> >>> On Fri, May 24, 2024 at 01:15:01PM +0800, quic_zijuhu wrote:
> >>>> On 5/24/2024 12:33 PM, Greg KH wrote:
> >>>>> On Fri, May 24, 2024 at 12:20:03PM +0800, Zijun Hu wrote:
> >>>>>> zap_modalias_env() wrongly calculates size of memory block
> >>>>>> to move, so maybe cause OOB memory access issue, fixed by
> >>>>>> correcting size to memmove.
> >>>>>
> >>>>> "maybe" or "does"?  That's a big difference :)
> >>>>>
> >>>> i found this issue by reading code instead of really meeting this issue.
> >>>> this issue should be prone to happen if there are more than 1 other
> >>>> environment vars.
> >>>
> >>> But does it?  Given that we have loads of memory checkers, and I haven't
> >>> ever seen any report of any overrun, it would be nice to be sure.
> >>>
> >> yes. if @env includes env vairable MODALIAS and  more than one other env
> >> vairables. then (env->buflen - len) must be greater that actual size of
> >> "target block" shown previously, so the OOB issue must happen.
> > 
> > Then why are none of the tools that we have for catching out-of-bound
> > issues triggered here?  Are the tools broken or is this really just not
> > ever happening?  It would be good to figure that out...
> > 
> don't know why. perhaps, need to report our case to expert of tools.

Try running them yourself and see!

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

[quic_zijuhu]

On 5/24/2024 7:47 PM, Greg KH wrote:
> On Fri, May 24, 2024 at 05:08:06PM +0800, quic_zijuhu wrote:
>> On 5/24/2024 2:56 PM, Greg KH wrote:
>>> On Fri, May 24, 2024 at 01:34:49PM +0800, quic_zijuhu wrote:
>>>> On 5/24/2024 1:21 PM, Greg KH wrote:
>>>>> On Fri, May 24, 2024 at 01:15:01PM +0800, quic_zijuhu wrote:
>>>>>> On 5/24/2024 12:33 PM, Greg KH wrote:
>>>>>>> On Fri, May 24, 2024 at 12:20:03PM +0800, Zijun Hu wrote:
>>>>>>>> zap_modalias_env() wrongly calculates size of memory block
>>>>>>>> to move, so maybe cause OOB memory access issue, fixed by
>>>>>>>> correcting size to memmove.
>>>>>>>
>>>>>>> "maybe" or "does"?  That's a big difference :)
>>>>>>>
>>>>>> i found this issue by reading code instead of really meeting this issue.
>>>>>> this issue should be prone to happen if there are more than 1 other
>>>>>> environment vars.
>>>>>
>>>>> But does it?  Given that we have loads of memory checkers, and I haven't
>>>>> ever seen any report of any overrun, it would be nice to be sure.
>>>>>
>>>> yes. if @env includes env vairable MODALIAS and  more than one other env
>>>> vairables. then (env->buflen - len) must be greater that actual size of
>>>> "target block" shown previously, so the OOB issue must happen.
>>>
>>> Then why are none of the tools that we have for catching out-of-bound
>>> issues triggered here?  Are the tools broken or is this really just not
>>> ever happening?  It would be good to figure that out...
>>>
>> don't know why. perhaps, need to report our case to expert of tools.
> 
> Try running them yourself and see!
i find out the reason why the OOB issue is difficult to be observed.
the reason is that MODALIAS is the last variable added by most of
drivers by accident, and it skips the obvious wrong logic within
zap_modalias_env().

you maybe run below command to confirm the reason.
grep -l -r MODALIAS drivers/  | xargs grep add_uevent_var

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

[Zhou congjie]

On Fri, 24 May 2024, Zijun Hu wrote:

> Subject: [PATCH] kobject_uevent: Fix OOB access within zap_modalias_env()
> zap_modalias_env() wrongly calculates size of memory block
> to move, so maybe cause OOB memory access issue, 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>
> ---
>  lib/kobject_uevent.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/kobject_uevent.c b/lib/kobject_uevent.c
> index 03b427e2707e..f153b4f9d4d9 100644
> --- a/lib/kobject_uevent.c
> +++ b/lib/kobject_uevent.c
> @@ -434,7 +434,7 @@ static void zap_modalias_env(struct kobj_uevent_env *env)
>  
>  		if (i != env->envp_idx - 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;
> 

I notice it too.

In the debug, I find that length of "env->buflen - len" is definitely 
larger than  "env->buf + env->buflen - env->envp[i+1". So memmove() just 
copy some extra '\0', and the problem will not happen when the length of 
env variables is much smaller than 2048. That is why the problem is 
difficult to be observed.

But when the length of env variables is close to 2048 or even more than 
2048, the memmove will access the memory not belong to env->buf[2048]. 

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

[quic_zijuhu]

On 6/30/2024 11:08 PM, Zhou congjie wrote:
> On Fri, 24 May 2024, Zijun Hu wrote:
> 
>> Subject: [PATCH] kobject_uevent: Fix OOB access within zap_modalias_env()
>> zap_modalias_env() wrongly calculates size of memory block
>> to move, so maybe cause OOB memory access issue, 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>
>> ---
>>  lib/kobject_uevent.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/lib/kobject_uevent.c b/lib/kobject_uevent.c
>> index 03b427e2707e..f153b4f9d4d9 100644
>> --- a/lib/kobject_uevent.c
>> +++ b/lib/kobject_uevent.c
>> @@ -434,7 +434,7 @@ static void zap_modalias_env(struct kobj_uevent_env *env)
>>  
>>  		if (i != env->envp_idx - 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;
>>
> 
> I notice it too.
> 
> In the debug, I find that length of "env->buflen - len" is definitely 
> larger than  "env->buf + env->buflen - env->envp[i+1". So memmove() just 
> copy some extra '\0', and the problem will not happen when the length of 
> env variables is much smaller than 2048. That is why the problem is 
> difficult to be observed.
>
yes, it is a factor of why this issue is not easy to be observed
> But when the length of env variables is close to 2048 or even more than 
> 2048, the memmove will access the memory not belong to env->buf[2048]. 
>