• Vincent Mailhol's avatar
    can: etas_es58x: rework the version check logic to silence -Wformat-truncation · 107e6f6f
    Vincent Mailhol authored
    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)
    
    Before starting to parse the product info string, set the version
    sub-numbers to the maximum unsigned integer thus violating the
    definitions of struct es58x_sw_version or struct es58x_hw_revision.
    
    Then, rework 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 by 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 checks are paranoid but have the merit to silence the
    newly introduced W=1 format-truncation warning [1].
    
    [1] commit 6d4ab2e9 ("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@mail.gmail.com/Reported-by: default avatarMarc Kleine-Budde <mkl@pengutronix.de>
    Closes: https://lore.kernel.org/linux-can/20230914-carrousel-wrecker-720a08e173e9-mkl@pengutronix.de/
    Fixes: 9f06631c ("can: etas_es58x: export product information through devlink_ops::info_get()")
    Signed-off-by: default avatarVincent Mailhol <mailhol.vincent@wanadoo.fr>
    Link: https://lore.kernel.org/all/20230924110914.183898-2-mailhol.vincent@wanadoo.frSigned-off-by: default avatarMarc Kleine-Budde <mkl@pengutronix.de>
    107e6f6f
es58x_devlink.c 7.69 KB