> +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. 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? Maybe consider
"xxx.xxx.xxx"?
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.
Andrew
Hi Andrew,
Thank you for the review and the interesting comments on the parsing.
On. 27 Nov. 2022 at 02:37, Andrew Lunn <[email protected]> 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/[email protected]/
).
> 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
> 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.
That is good enough for me.
> I do not care any more as long as it does not result in
> undefined behaviour.
Agreed. Hopefully sscanf cannot go completely wrong and go off the end
of the buffer. That i would care about. Bit i guess the USB fuzzers
would of hit such problems already.
Andrew
On Mon. 28 Nov. 2022 at 00:08, Andrew Lunn <[email protected]> wrote:
> > 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.
>
> That is good enough for me.
>
> > I do not care any more as long as it does not result in
> > undefined behaviour.
>
> Agreed. Hopefully sscanf cannot go completely wrong and go off the end
> of the buffer. That i would care about. Bit i guess the USB fuzzers
> would of hit such problems already.
On the surface, the sscanf() seems OK. It will break the while loop
when reaching the end of the format:
https://elixir.bootlin.com/linux/v6.1-rc6/source/lib/vsprintf.c#L3429
or the end of the string:
https://elixir.bootlin.com/linux/v6.1-rc6/source/lib/vsprintf.c#L3501
(I am skipping details here, there are other branches that will break
the while loop and all of them look good).
And me not being the first person using sscanf(), I hope that if a bug
existed, it would have already been spotted by some static
analysis/fuzzing/code review :)
That said, I think I answered all your comments. Can I get your
reviewed-by or ack tag? Thank you!
Yours sincerely,
Vincent Mailhol
> That said, I think I answered all your comments. Can I get your
> reviewed-by or ack tag? Thank you!
Reviewed-by: Andrew Lunn <[email protected]>
Andrew