From: Zijun Hu <quic_zijuhu@quicinc.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).
Fixes: dd27dcda37f0 ("of/device: merge of_device_uevent")
Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.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],
- 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;
env->buflen += sl;
--
2.34.1On Thu, Dec 5, 2024 at 6:53 PM Zijun Hu <zijun_hu@icloud.com> wrote:
>
> From: Zijun Hu <quic_zijuhu@quicinc.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.
>
> Fixes: dd27dcda37f0 ("of/device: merge of_device_uevent")
> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.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.
> - 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.
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:
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;
Rob
On 2024/12/10 04:34, Rob Herring wrote:
> On Thu, Dec 5, 2024 at 6:53 PM Zijun Hu <zijun_hu@icloud.com> wrote:
>>
>> From: Zijun Hu <quic_zijuhu@quicinc.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@quicinc.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
On Tue, Dec 10, 2024 at 6:39 AM Zijun Hu <zijun_hu@icloud.com> wrote:
>
> On 2024/12/10 04:34, Rob Herring wrote:
> > On Thu, Dec 5, 2024 at 6:53 PM Zijun Hu <zijun_hu@icloud.com> wrote:
> >>
> >> From: Zijun Hu <quic_zijuhu@quicinc.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@quicinc.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'.
Ah, right. However, we still end up with a truncated value though it
is nul terminated.
> 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.
There is one major difference. add_uevent_var() will not output
anything if the whole string doesn't fit. Whereas we might output a
truncated value because the add_uevent_var() call updated env->buflen
and env->envp_idx. We could unwind that I suppose, but that involves
even more mucking with the internals of the env struct.
> > 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() ?
Yes.
>
> 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.
Yes, that can be simplified to just a kasnprintf() call.
Rob
On 2024/12/10 22:10, Rob Herring wrote: > Ah, right. However, we still end up with a truncated value though it > is nul terminated. > >> 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. > There is one major difference. add_uevent_var() will not output > anything if the whole string doesn't fit. Whereas we might output a > truncated value because the add_uevent_var() call updated env->buflen > and env->envp_idx. We could unwind that I suppose, but that involves > even more mucking with the internals of the env struct. you are right. i would like to try to solve involved issue in next revision. (^^)
© 2016 - 2025 Red Hat, Inc.