Following [1], es58x_devlink.c now triggers the following
format-truncation GCC warnings:
drivers/net/can/usb/etas_es58x/es58x_devlink.c: In function ‘es58x_devlink_info_get’:
drivers/net/can/usb/etas_es58x/es58x_devlink.c:201:41: warning: ‘%02u’ directive output may be truncated writing between 2 and 3 bytes into a region of size between 1 and 3 [-Wformat-truncation=]
201 | snprintf(buf, sizeof(buf), "%02u.%02u.%02u",
| ^~~~
drivers/net/can/usb/etas_es58x/es58x_devlink.c:201:30: note: directive argument in the range [0, 255]
201 | snprintf(buf, sizeof(buf), "%02u.%02u.%02u",
| ^~~~~~~~~~~~~~~~
drivers/net/can/usb/etas_es58x/es58x_devlink.c:201:3: note: ‘snprintf’ output between 9 and 12 bytes into a destination of size 9
201 | snprintf(buf, sizeof(buf), "%02u.%02u.%02u",
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
202 | fw_ver->major, fw_ver->minor, fw_ver->revision);
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/net/can/usb/etas_es58x/es58x_devlink.c:211:41: warning: ‘%02u’ directive output may be truncated writing between 2 and 3 bytes into a region of size between 1 and 3 [-Wformat-truncation=]
211 | snprintf(buf, sizeof(buf), "%02u.%02u.%02u",
| ^~~~
drivers/net/can/usb/etas_es58x/es58x_devlink.c:211:30: note: directive argument in the range [0, 255]
211 | snprintf(buf, sizeof(buf), "%02u.%02u.%02u",
| ^~~~~~~~~~~~~~~~
drivers/net/can/usb/etas_es58x/es58x_devlink.c:211:3: note: ‘snprintf’ output between 9 and 12 bytes into a destination of size 9
211 | snprintf(buf, sizeof(buf), "%02u.%02u.%02u",
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
212 | bl_ver->major, bl_ver->minor, bl_ver->revision);
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/net/can/usb/etas_es58x/es58x_devlink.c:221:38: warning: ‘%03u’ directive output may be truncated writing between 3 and 5 bytes into a region of size between 2 and 4 [-Wformat-truncation=]
221 | snprintf(buf, sizeof(buf), "%c%03u/%03u",
| ^~~~
drivers/net/can/usb/etas_es58x/es58x_devlink.c:221:30: note: directive argument in the range [0, 65535]
221 | snprintf(buf, sizeof(buf), "%c%03u/%03u",
| ^~~~~~~~~~~~~
drivers/net/can/usb/etas_es58x/es58x_devlink.c:221:3: note: ‘snprintf’ output between 9 and 13 bytes into a destination of size 9
221 | snprintf(buf, sizeof(buf), "%c%03u/%03u",
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
222 | hw_rev->letter, hw_rev->major, hw_rev->minor);
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
This is not an actual bug because the sscanf() parsing makes sure that
the u8 are only two digits long and the u16 only three digits long.
Thus below declaration:
char buf[max(sizeof("xx.xx.xx"), sizeof("axxx/xxx"))];
allocates just what is needed to represent either of the versions.
This warning was known but ignored because, at the time of writing,
-Wformat-truncation was not present in the kernel, not even at W=3 [2].
One way to silence this warning is to check the range of all sub
version numbers are valid: [0, 99] for u8 and range [0, 999] for u16.
The module already has a logic which considers that when all the sub
version numbers are zero, the version number is not set. Note that not
having access to the device specification, this was an arbitrary
decision. This logic can thus be removed in favor of global check that
would cover both cases:
- the version number is not set (parsing failed)
- the version number is not valid (paranoiac check to please gcc)
When the parsing fails, set the versions number to the maximum
unsigned integer thus violating the definitions of struct
es58x_sw_version or struct es58x_hw_revision.
Then, rework how the es58x_sw_version_is_set() and
es58x_hw_revision_is_set() functions: remove the check that the
sub-numbers are non zero and replace it to a check that they fit in
the expected number of digits. This done, rename the functions to
reflect the change and rewrite the documentation. While doing so, also
add a description of the return value.
Finally, the previous version only checked that
&es58x_hw_revision.letter was not the null character. Replace this
check by an alphanumeric character check to make sure that we never
return a special character or a non-printable one and update the
documentation of struct es58x_hw_revision accordingly.
All those extra verifications are paranoid checks but have the merit
to silence the newly introduced W=1 format-truncation warning [1].
[1] commit 6d4ab2e97dcf ("extrawarn: enable format and stringop overflow warnings in W=1")
Link: https://git.kernel.org/torvalds/c/6d4ab2e97dcf
[2] https://lore.kernel.org/all/CAMZ6Rq+K+6gbaZ35SOJcR9qQaTJ7KR0jW=XoDKFkobjhj8CHhw@xxxxxxxxxxxxxx/
Reported-by: Marc Kleine-Budde <mkl@xxxxxxxxxxxxxx>
Closes: https://lore.kernel.org/linux-can/20230914-carrousel-wrecker-720a08e173e9-mkl@xxxxxxxxxxxxxx/
Fixes: 9f06631c3f1f ("can: etas_es58x: export product information through devlink_ops::info_get()")
Signed-off-by: Vincent Mailhol <mailhol.vincent@xxxxxxxxxx>
---
drivers/net/can/usb/etas_es58x/es58x_core.h | 6 +-
.../net/can/usb/etas_es58x/es58x_devlink.c | 61 +++++++++++++------
2 files changed, 45 insertions(+), 22 deletions(-)
diff --git a/drivers/net/can/usb/etas_es58x/es58x_core.h b/drivers/net/can/usb/etas_es58x/es58x_core.h
index c1ba1a4e8857..f068e47fbc72 100644
--- a/drivers/net/can/usb/etas_es58x/es58x_core.h
+++ b/drivers/net/can/usb/etas_es58x/es58x_core.h
@@ -378,13 +378,13 @@ struct es58x_sw_version {
/**
* struct es58x_hw_revision - Hardware revision number.
- * @letter: Revision letter.
+ * @letter: Revision letter, an alphanumeric character.
* @major: Version major number, represented on three digits.
* @minor: Version minor number, represented on three digits.
*
* The hardware revision uses its own format: "axxx/xxx" where 'a' is
- * a letter and 'x' a digit. It can be retrieved from the product
- * information string.
+ * an alphanumeric character and 'x' a digit. It can be retrieved from
+ * the product information string.
*/
struct es58x_hw_revision {
char letter;
diff --git a/drivers/net/can/usb/etas_es58x/es58x_devlink.c b/drivers/net/can/usb/etas_es58x/es58x_devlink.c
index 9fba29e2f57c..edd35ba01fb1 100644
--- a/drivers/net/can/usb/etas_es58x/es58x_devlink.c
+++ b/drivers/net/can/usb/etas_es58x/es58x_devlink.c
@@ -34,7 +34,8 @@
*
* Parse @prod_info and store the version number in
* &es58x_dev.firmware_version or &es58x_dev.bootloader_version
- * according to @prefix value.
+ * according to @prefix value. If the parsing fails, set an invalid
+ * version number.
*
* Return: zero on success, -EINVAL if @prefix contains an invalid
* value and -EBADMSG if @prod_info could not be parsed.
@@ -42,6 +43,11 @@
static int es58x_parse_sw_version(struct es58x_device *es58x_dev,
const char *prod_info, const char *prefix)
{
+ static const struct es58x_sw_version sw_version_not_set = {
+ .major = -1,
+ .minor = -1,
+ .revision = -1,
+ };
struct es58x_sw_version *version;
int major, minor, revision;
@@ -63,8 +69,10 @@ static int es58x_parse_sw_version(struct es58x_device *es58x_dev,
return -EBADMSG;
}
- if (sscanf(prod_info, "%2u.%2u.%2u", &major, &minor, &revision) != 3)
+ if (sscanf(prod_info, "%2u.%2u.%2u", &major, &minor, &revision) != 3) {
+ *version = sw_version_not_set;
return -EBADMSG;
+ }
version->major = major;
version->minor = minor;
@@ -85,7 +93,8 @@ static int es58x_parse_sw_version(struct es58x_device *es58x_dev,
* and 'x' a digit.
*
* Parse @prod_info and store the hardware revision number in
- * &es58x_dev.hardware_revision.
+ * &es58x_dev.hardware_revision. If the parsing fails, set an invalid
+ * revision number.
*
* Return: zero on success, -EBADMSG if @prod_info could not be
* parsed.
@@ -93,6 +102,11 @@ static int es58x_parse_sw_version(struct es58x_device *es58x_dev,
static int es58x_parse_hw_rev(struct es58x_device *es58x_dev,
const char *prod_info)
{
+ static const struct es58x_hw_revision hw_revision_not_set = {
+ .letter = '\0',
+ .major = -1,
+ .minor = -1,
+ };
char letter;
int major, minor;
@@ -106,8 +120,10 @@ static int es58x_parse_hw_rev(struct es58x_device *es58x_dev,
return -EBADMSG;
prod_info++;
- if (sscanf(prod_info, "%c%3u/%3u", &letter, &major, &minor) != 3)
+ if (sscanf(prod_info, "%c%3u/%3u", &letter, &major, &minor) != 3) {
+ es58x_dev->hardware_revision = hw_revision_not_set;
return -EBADMSG;
+ }
es58x_dev->hardware_revision.letter = letter;
es58x_dev->hardware_revision.major = major;
@@ -150,29 +166,36 @@ void es58x_parse_product_info(struct es58x_device *es58x_dev)
}
/**
- * es58x_sw_version_is_set() - Check if the version is a valid number.
+ * es58x_sw_version_is_valid() - Check if the version is a valid number.
* @sw_ver: Version number of either the firmware or the bootloader.
*
- * If &es58x_sw_version.major, &es58x_sw_version.minor and
- * &es58x_sw_version.revision are all zero, the product string could
- * not be parsed and the version number is invalid.
+ * If any of the software version sub-numbers do not fit on two
+ * digits, the version is invalid, most probably because the product
+ * string could not be parsed.
+ *
+ * Return: @true if the software version is valid, @false otherwise.
*/
-static inline bool es58x_sw_version_is_set(struct es58x_sw_version *sw_ver)
+static inline bool es58x_sw_version_is_valid(struct es58x_sw_version *sw_ver)
{
- return sw_ver->major || sw_ver->minor || sw_ver->revision;
+ return sw_ver->major < 100 && sw_ver->minor < 100 &&
+ sw_ver->revision < 100;
}
/**
- * es58x_hw_revision_is_set() - Check if the revision is a valid number.
+ * es58x_hw_revision_is_valid() - Check if the revision is a valid number.
* @hw_rev: Revision number of the hardware.
*
- * If &es58x_hw_revision.letter is the null character, the product
- * string could not be parsed and the hardware revision number is
- * invalid.
+ * If &es58x_hw_revision.letter is not a alphanumeric character or if
+ * any of the hardware revision sub-numbers do not fit on three
+ * digits, the revision is invalid, most probably because the product
+ * string could not be parsed.
+ *
+ * Return: @true if the hardware revision is valid, @false otherwise.
*/
-static inline bool es58x_hw_revision_is_set(struct es58x_hw_revision *hw_rev)
+static inline bool es58x_hw_revision_is_valid(struct es58x_hw_revision *hw_rev)
{
- return hw_rev->letter != '\0';
+ return isalnum(hw_rev->letter) && hw_rev->major < 1000 &&
+ hw_rev->minor < 1000;
}
/**
@@ -197,7 +220,7 @@ static int es58x_devlink_info_get(struct devlink *devlink,
char buf[max(sizeof("xx.xx.xx"), sizeof("axxx/xxx"))];
int ret = 0;
- if (es58x_sw_version_is_set(fw_ver)) {
+ if (es58x_sw_version_is_valid(fw_ver)) {
snprintf(buf, sizeof(buf), "%02u.%02u.%02u",
fw_ver->major, fw_ver->minor, fw_ver->revision);
ret = devlink_info_version_running_put(req,
@@ -207,7 +230,7 @@ static int es58x_devlink_info_get(struct devlink *devlink,
return ret;
}
- if (es58x_sw_version_is_set(bl_ver)) {
+ if (es58x_sw_version_is_valid(bl_ver)) {
snprintf(buf, sizeof(buf), "%02u.%02u.%02u",
bl_ver->major, bl_ver->minor, bl_ver->revision);
ret = devlink_info_version_running_put(req,
@@ -217,7 +240,7 @@ static int es58x_devlink_info_get(struct devlink *devlink,
return ret;
}
- if (es58x_hw_revision_is_set(hw_rev)) {
+ if (es58x_hw_revision_is_valid(hw_rev)) {
snprintf(buf, sizeof(buf), "%c%03u/%03u",
hw_rev->letter, hw_rev->major, hw_rev->minor);
ret = devlink_info_version_fixed_put(req,
--
2.25.1
