linux-kernel - Re: [PATCH 05/10] of: Fix available buffer size calculating error in API of_device_uevent

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <14fe473e-5f56-4a61-899c-bbb79e2aed3b@icloud.com>
Date: Tue, 10 Dec 2024 20:39:27 +0800
From: Zijun Hu <zijun_hu@...oud.com>
To: Rob Herring <robh@...nel.org>
Cc: Saravana Kannan <saravanak@...gle.com>,
 Leif Lindholm <leif.lindholm@...aro.org>,
 Stephen Boyd <stephen.boyd@...aro.org>, Maxime Ripard <mripard@...nel.org>,
 Robin Murphy <robin.murphy@....com>, Grant Likely
 <grant.likely@...retlab.ca>, devicetree@...r.kernel.org,
 linux-kernel@...r.kernel.org, Zijun Hu <quic_zijuhu@...cinc.com>
Subject: Re: [PATCH 05/10] of: Fix available buffer size calculating error in
 API of_device_uevent_modalias()

On 2024/12/10 04:34, Rob Herring wrote:
> On Thu, Dec 5, 2024 at 6:53 PM Zijun Hu <zijun_hu@...oud.com> wrote:
>>
>> From: Zijun Hu <quic_zijuhu@...cinc.com>
>>
>> of_device_uevent_modalias() saves MODALIAS value from offset
>> (@env->buflen - 1), so the available buffer size should be
>> (sizeof(@env->buf) - @env->buflen + 1), but it uses the wrong
>> size (sizeof(@env->buf) - @env->buflen).
>>
>> Fix by using right size (sizeof(@env->buf) - @env->buflen + 1).
> 
> Just writing what the diff says already is not that useful. The key
> part you need to know is why we back up by 1 character to begin with.
> 

will correct commit message in v2.

>>
>> Fixes: dd27dcda37f0 ("of/device: merge of_device_uevent")
>> Signed-off-by: Zijun Hu <quic_zijuhu@...cinc.com>
>> ---
>>  drivers/of/device.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/of/device.c b/drivers/of/device.c
>> index edf3be1972658f6dc165f577da53b10c7eebc116..ee29c07c83b9e6abd9b1c7747dd341026bc79eb0 100644
>> --- a/drivers/of/device.c
>> +++ b/drivers/of/device.c
>> @@ -266,10 +266,10 @@ int of_device_uevent_modalias(const struct device *dev, struct kobj_uevent_env *
>>                 return -ENOMEM;
>>
>>         sl = of_modalias(dev->of_node, &env->buf[env->buflen-1],
> 
> This could use a comment why we back up by 1. Better to put it in a
> variable than add/subtract 1 everywhere:
> 
> /* After add_uevent_event(), buflen is at character after the nul char
> which needs to be overwritten */
> buflen = env->buflen - 1;
> 
> And then use 'buflen' throughout.
> 

good proposal. may use it for v2 after discussion done.

>> -                        sizeof(env->buf) - env->buflen);
>> +                        sizeof(env->buf) - env->buflen + 1);
>>         if (sl < 0)
>>                 return sl;
>> -       if (sl >= (sizeof(env->buf) - env->buflen))
>> +       if (sl >= (sizeof(env->buf) - env->buflen + 1))
>>                 return -ENOMEM;
> 
> There's another potential problem. If we return before here, we end up
> with "OF_MODALIAS=\0" or "OF_MODALIAS=some-non-nul-terminated-str".
> Maybe that doesn't matter? I haven't looked at the caller.
> 

that does not matter since current logic follows below 2 rules

1) all strings in @env->buf always terminated with '\0'.
2) both env->buflen and env->envp_idx are not updated once @env->buf
does not enough spaces then failed.

current logic has no difference with normal add_uevent_var() usage.

> I think a better solution to all this would be making add_uevent_var
> work to construct the full value. We could add "%pOFm" format that
> calls of_mod_alias(). Then this function becomes just:
> 
of_modalias() ?

agree with you.
for good practice, users should only use uevent APIs and should not
depend on  uevent's internal implementation.

> return add_uevent_var(env, "MODALIAS=%pOFm");
> 
> And of_device_modalias() can be:
> 
> sl = snprintf(str, len, "%pOFm\n", dev->of_node);
> if (sl >= len)
>   return -ENOMEM;
> return sl;
> 

looks good.

of_request_module() provides another solution.

> Rob