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

From: Vincent MAILHOL <mailhol.vincent@wanadoo.fr>
To: Andrew Lunn <andrew@lunn.ch>
Cc: linux-can@vger.kernel.org, Marc Kleine-Budde <mkl@pengutronix.de>,
	linux-kernel@vger.kernel.org,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	netdev@vger.kernel.org, linux-usb@vger.kernel.org,
	Saeed Mahameed <saeed@kernel.org>, Jiri Pirko <jiri@nvidia.com>,
	Lukas Magel <lukas.magel@posteo.net>
Subject: Re: [PATCH v4 3/6] can: etas_es58x: export product information through devlink_ops::info_get()
Date: Sun, 27 Nov 2022 13:31:17 +0900	[thread overview]
Message-ID: <CAMZ6Rq+-RH4QhBkdm_y60U1AaqEON5Mbii9N9p=zT_2AMY9gmw@mail.gmail.com> (raw)
In-Reply-To: <CAMZ6Rq+K+6gbaZ35SOJcR9qQaTJ7KR0jW=XoDKFkobjhj8CHhw@mail.gmail.com>

On Sun. 27 Nov. 2022 at 12:42, Vincent MAILHOL
<mailhol.vincent@wanadoo.fr> wrote:
> Hi Andrew,
>
> Thank you for the review and the interesting comments on the parsing.
>
> On. 27 Nov. 2022 at 02:37, Andrew Lunn <andrew@lunn.ch> 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@wanadoo.fr/
> ).
>
> > 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.

Never mind. I forgot that I already accounted for that. The "%2u"
format in sscanf() will detect if the number is three or more digits.
So I am thinking of leaving everything as-is.

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