lkml.org 
[lkml]   [2022]   [Mar]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH v5 01/11] driver: platform: Add helper for safer setting of driver_override
On Wed, Mar 16, 2022 at 5:06 PM Krzysztof Kozlowski
<krzysztof.kozlowski@canonical.com> wrote:

...

> +int driver_set_override(struct device *dev, const char **override,
> + const char *s, size_t len)
> +{
> + const char *new, *old;
> + char *cp;

> + if (!dev || !override || !s)
> + return -EINVAL;

Sorry, I didn't pay much attention on this. First of all, I would drop
dev checks and simply require that dev should be valid. Do you expect
this can be called when dev is invalid? I would like to hear if it's
anything but theoretical. Second one, is the !s requirement. Do I
understand correctly that the string must be always present? But then
how we NULify the override? Is it possible? Third one is absence of
len check. See below.

> + /*
> + * The stored value will be used in sysfs show callback (sysfs_emit()),
> + * which has a length limit of PAGE_SIZE and adds a trailing newline.
> + * Thus we can store one character less to avoid truncation during sysfs
> + * show.
> + */
> + if (len >= (PAGE_SIZE - 1))
> + return -EINVAL;

I would relax this to make sure we can use it if \n is within this limit.

> + cp = strnchr(s, len, '\n');
> + if (cp)
> + len = cp - s;
> +
> + new = kstrndup(s, len, GFP_KERNEL);

Here is a word about the len check.

> + if (!new)

If len == 0, this won't trigger and you have something very
interesting as a result.

One way is to use ZERO_PTR_OR_NULL() another is explicitly check for 0
and issue a (different?) error code.

> + return -ENOMEM;
> +
> + device_lock(dev);
> + old = *override;
> + if (cp != s) {
> + *override = new;
> + } else {
> + kfree(new);
> + *override = NULL;
> + }
> + device_unlock(dev);
> +
> + kfree(old);
> +
> + return 0;
> +}

--
With Best Regards,
Andy Shevchenko

\
 
 \ /
  Last update: 2022-03-16 16:55    [W:0.140 / U:0.160 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site