Re: [PATCH 05/10] of: Fix available buffer size calculating error in API of_device_uevent_modalias()

[Rob Herring]

On Thu, Dec 5, 2024 at 6:53 PM Zijun Hu <zijun_hu@xxxxxxxxxx> wrote:
>
> From: Zijun Hu <quic_zijuhu@xxxxxxxxxxx>
>
> 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@xxxxxxxxxxx>
> ---
>  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