Re: [PATCH v4 3/6] can: etas_es58x: export product information through devlink_ops::info_get()

[Vincent MAILHOL]

Hi Andrew,

Thank you for the review and the interesting comments on the parsing.

On. 27 Nov. 2022 at 02:37, Andrew Lunn <andrew@xxxxxxx> wrote:
> > +struct es58x_sw_version {
> > +     u8 major;
> > +     u8 minor;
> > +     u8 revision;
> > +};
>
> > +static int es58x_devlink_info_get(struct devlink *devlink,
> > +                               struct devlink_info_req *req,
> > +                               struct netlink_ext_ack *extack)
> > +{
> > +     struct es58x_device *es58x_dev = devlink_priv(devlink);
> > +     struct es58x_sw_version *fw_ver = &es58x_dev->firmware_version;
> > +     struct es58x_sw_version *bl_ver = &es58x_dev->bootloader_version;
> > +     struct es58x_hw_revision *hw_rev = &es58x_dev->hardware_revision;
> > +     char buf[max(sizeof("xx.xx.xx"), sizeof("axxx/xxx"))];
> > +     int ret = 0;
> > +
> > +     if (es58x_sw_version_is_set(fw_ver)) {
> > +             snprintf(buf, sizeof(buf), "%02u.%02u.%02u",
> > +                      fw_ver->major, fw_ver->minor, fw_ver->revision);
>
> I see you have been very careful here, but i wonder if you might still
> get some compiler/static code analyser warnings here. As far as i
> remember %02u does not limit it to two characters.

I checked, none of gcc and clang would trigger a warning even for a
'make W=12'. More generally speaking, I made sure that my driver is
free of any W=12.
(except from the annoying spam from GENMASK_INPUT_CHECK for which my
attempts to silence it were rejected:
https://lore.kernel.org/all/20220426161658.437466-1-mailhol.vincent@xxxxxxxxxx/
).

> If the number is
> bigger than 99, it will take three characters. And your types are u8,
> so the compiler could consider these to be 3 characters each. So you
> end up truncating. Which you look to of done correctly, but i wonder
> if some over zealous checker will report it?

That zealous check is named -Wformat-truncation in gcc (I did not find
it in clang). Even W=3 doesn't report it so I consider this to be
fine.

> Maybe consider "xxx.xxx.xxx"?

If I do that, I also need to consider the maximum length of the
hardware revision would be "a/xxxxx/xxxxx" because the numbers are
u16. The declaration would become:

        char buf[max(sizeof("xxx.xxx.xxx"), sizeof("axxxxx/xxxxx"))];

Because no such warning exists in the kernel, I do not think the above
line to be a good trade off. I would like to keep things as they are,
it is easier to read. That said, I will add an extra check in
es58x_parse_sw_version() and es58x_parse_hw_revision() to assert that
the number are not bigger than 99 for the software version (and not
bigger than 999 for the hardware revision). That way the code will
guarantee that the truncation can never occur.

> Nice paranoid code by the way. I'm not the best at spotting potential
> buffer overflows, but this code looks good. The only question i had
> left was how well sscanf() deals with UTF-8.

It does not consider UTF-8. The %u is a _parse_integer_limit() in disguise.
  https://elixir.bootlin.com/linux/v6.1-rc6/source/lib/vsprintf.c#L3637
  https://elixir.bootlin.com/linux/v6.1-rc6/source/lib/vsprintf.c#L70

_parse_integer_limit() just check for ASCII digits so the first UTF-8
character would make the function return.
  https://elixir.bootlin.com/linux/v6.1-rc6/source/lib/kstrtox.c#L65

For example, this:
  "ＦＷ:03.00.06"
would fail parsing because sscanf() will not be able to match the
first byte of the UTF-8 'Ｆ' with 'F'.

Another example:
  "FW:０３.００.０６"
would also fail parsing because _parse_integer_limit() will not
recognize the first byte of UTF-8 '０' as a valid ASCII digit and
return early.

To finish, a very edge case:
  "FW:03.00.0６"
would incorrectly succeed. It will parse "FW:03.00.0" successfully and
will return when encountering the UTF-8 '６'. But I am not willing to
cover that edge case. If the device goes into this level of
perversion, I do not care any more as long as it does not result in
undefined behaviour.


Yours sincerely,
Vincent Mailhol