Re: [PATCH v5 01/11] driver: platform: Add helper for safer setting of driver_override

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

On 16/03/2022 16:54, Andy Shevchenko wrote:
> 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;

I focused on some other topics, so I did not respond to your questions
for some time.

> 
>> +       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?

No, I can skip the check.

> 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?

I did not change the convention of this sysfs hook, so removing of
override is passing empty string "". Have in mind that his interface is
mainly for sysfs, not for other drivers.

> 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.

Relax in what way? Store more than PAGE_SIZE? This is a user-visible
string and the name of a driver.

> 
>> +       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.

True, empty string would be set as override. The API says that empty
string clears the override, so len==0 should be valid (just like "\n" is
ok).

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



Best regards,
Krzysztof