| Message ID | 0278c7dfbc23f78a2d85060369132782f8466090.1698007858.git.christophe.jaillet@wanadoo.fr (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | liquidio: Fix an off by one in octeon_download_firmware() | expand |
| Context | Check | Description |
|---|---|---|
| netdev/series_format | success | Posting correctly formatted |
| netdev/tree_selection | success | Clearly marked for net |
| netdev/fixes_present | success | Fixes tag present in non-next series |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | success | Errors and warnings before: 9 this patch: 9 |
| netdev/cc_maintainers | success | CCed 8 of 8 maintainers |
| netdev/build_clang | success | Errors and warnings before: 1386 this patch: 1386 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/deprecated_api | success | None detected |
| netdev/check_selftest | success | No net selftest shell script |
| netdev/verify_fixes | success | No Fixes tag |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 1386 this patch: 1386 |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 40 lines checked |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
On 10/22/2023 1:59 PM, Christophe JAILLET wrote: > In order to remove the usage of strncat(), write directly at the rigth > place in the 'h->bootcmd' array and check if the output is truncated. > > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> > --- > The goal is to potentially remove the strncat() function from the kernel. > Their are only few users and most of them use it wrongly. > > This patch is compile tested only. > --- > .../net/ethernet/cavium/liquidio/octeon_console.c | 13 +++++-------- > 1 file changed, 5 insertions(+), 8 deletions(-) > > diff --git a/drivers/net/ethernet/cavium/liquidio/octeon_console.c b/drivers/net/ethernet/cavium/liquidio/octeon_console.c > index bd6baf2872a5..f1f0d7a0309a 100644 > --- a/drivers/net/ethernet/cavium/liquidio/octeon_console.c > +++ b/drivers/net/ethernet/cavium/liquidio/octeon_console.c > @@ -802,19 +802,17 @@ static int octeon_console_read(struct octeon_device *oct, u32 console_num, > } > > #define FBUF_SIZE (4 * 1024 * 1024) > -#define MAX_BOOTTIME_SIZE 80 > > int octeon_download_firmware(struct octeon_device *oct, const u8 *data, > size_t size) > { > struct octeon_firmware_file_header *h; > - char boottime[MAX_BOOTTIME_SIZE]; > struct timespec64 ts; > u32 crc32_result; > + u32 i, rem, used; > u64 load_addr; > u32 image_len; > int ret = 0; > - u32 i, rem; > > if (size < sizeof(struct octeon_firmware_file_header)) { > dev_err(&oct->pci_dev->dev, "Firmware file too small (%d < %d).\n", > @@ -896,16 +894,15 @@ int octeon_download_firmware(struct octeon_device *oct, const u8 *data, > * Octeon always uses UTC time. so timezone information is not sent. > */ > ktime_get_real_ts64(&ts); > - ret = snprintf(boottime, MAX_BOOTTIME_SIZE, > + > + used = strnlen(h->bootcmd, sizeof(h->bootcmd)); > + ret = snprintf(h->bootcmd + used, sizeof(h->bootcmd) - used, > " time_sec=%lld time_nsec=%ld", > (s64)ts.tv_sec, ts.tv_nsec); Maybe I am missing something here, but why not combine the boottime with the original write of bootcmd? I guess this is being updated periodically? The overall solution used here feels like the wrong way to do it... I guess the bootcmd is setup else where but I couldn't find the relevant code for that. What you did seems ok to me, but it still feels like there is a simpler way to achieve the desired result. Thanks, Jake
On Sun, 2023-10-22 at 22:59 +0200, Christophe JAILLET wrote: > In order to remove the usage of strncat(), write directly at the rigth > place in the 'h->bootcmd' array and check if the output is truncated. > > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> > --- > The goal is to potentially remove the strncat() function from the kernel. > Their are only few users and most of them use it wrongly. > > This patch is compile tested only. Then just switch to strlcat, would be less invasive. Thanks, Paolo
On Tue, Oct 24, 2023 at 01:11:13PM +0200, Paolo Abeni wrote: > On Sun, 2023-10-22 at 22:59 +0200, Christophe JAILLET wrote: > > In order to remove the usage of strncat(), write directly at the rigth > > place in the 'h->bootcmd' array and check if the output is truncated. > > > > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> > > --- > > The goal is to potentially remove the strncat() function from the kernel. > > Their are only few users and most of them use it wrongly. > > > > This patch is compile tested only. > > Then just switch to strlcat, would be less invasive. Linus was just complaining about strl* functions. https://lore.kernel.org/all/CAHk-=wj4BZei4JTiX9qsAwk8PEKnPrvkG5FU0i_HNkcDpy7NGQ@mail.gmail.com/ strlcat() does a strlen(src) so it's BROKEN BY DESIGN as Linus puts it. The advantage of strlcat() is that it always puts a NUL terminator in the dest buffer, but the disadvantage is that it introduces a read overflow. I would probably have written it like this: diff --git a/drivers/net/ethernet/cavium/liquidio/octeon_console.c b/drivers/net/ethernet/cavium/liquidio/octeon_console.c index 67c3570f875f..ebe9f7694d8b 100644 --- a/drivers/net/ethernet/cavium/liquidio/octeon_console.c +++ b/drivers/net/ethernet/cavium/liquidio/octeon_console.c @@ -899,13 +899,16 @@ int octeon_download_firmware(struct octeon_device *oct, const u8 *data, ret = snprintf(boottime, MAX_BOOTTIME_SIZE, " time_sec=%lld time_nsec=%ld", (s64)ts.tv_sec, ts.tv_nsec); - if ((sizeof(h->bootcmd) - strnlen(h->bootcmd, sizeof(h->bootcmd))) < - ret) { + + len = strnlen(h->bootcmd, sizeof(h->bootcmd)); + len += snprintf(h->bootcmd + len, sizeof(h->bootcmd) - len, + " time_sec=%lld time_nsec=%ld", + (s64)ts.tv_sec, ts.tv_nsec); + if (len >= sizeof(h->bootcmd)) { + h->bootcmd[orig] = '\0'; dev_err(&oct->pci_dev->dev, "Boot command buffer too small\n"); return -EINVAL; } - strncat(h->bootcmd, boottime, - sizeof(h->bootcmd) - strnlen(h->bootcmd, sizeof(h->bootcmd))); dev_info(&oct->pci_dev->dev, "Writing boot command: %s\n", h->bootcmd); Don't involve the "ret" variable. Just len +=. In the original code, if there wasn't enough space they truncated it before the " time_sec=%lld time_nsec=%ld" but keeping that behavior seems needlessly complicated. They already created one bug by complicating stuff. regards, dan carpenter
diff --git a/drivers/net/ethernet/cavium/liquidio/octeon_console.c b/drivers/net/ethernet/cavium/liquidio/octeon_console.c index bd6baf2872a5..f1f0d7a0309a 100644 --- a/drivers/net/ethernet/cavium/liquidio/octeon_console.c +++ b/drivers/net/ethernet/cavium/liquidio/octeon_console.c @@ -802,19 +802,17 @@ static int octeon_console_read(struct octeon_device *oct, u32 console_num, } #define FBUF_SIZE (4 * 1024 * 1024) -#define MAX_BOOTTIME_SIZE 80 int octeon_download_firmware(struct octeon_device *oct, const u8 *data, size_t size) { struct octeon_firmware_file_header *h; - char boottime[MAX_BOOTTIME_SIZE]; struct timespec64 ts; u32 crc32_result; + u32 i, rem, used; u64 load_addr; u32 image_len; int ret = 0; - u32 i, rem; if (size < sizeof(struct octeon_firmware_file_header)) { dev_err(&oct->pci_dev->dev, "Firmware file too small (%d < %d).\n", @@ -896,16 +894,15 @@ int octeon_download_firmware(struct octeon_device *oct, const u8 *data, * Octeon always uses UTC time. so timezone information is not sent. */ ktime_get_real_ts64(&ts); - ret = snprintf(boottime, MAX_BOOTTIME_SIZE, + + used = strnlen(h->bootcmd, sizeof(h->bootcmd)); + ret = snprintf(h->bootcmd + used, sizeof(h->bootcmd) - used, " time_sec=%lld time_nsec=%ld", (s64)ts.tv_sec, ts.tv_nsec); - if ((sizeof(h->bootcmd) - strnlen(h->bootcmd, sizeof(h->bootcmd))) <= - ret) { + if (ret >= sizeof(h->bootcmd) - used) { dev_err(&oct->pci_dev->dev, "Boot command buffer too small\n"); return -EINVAL; } - strncat(h->bootcmd, boottime, - sizeof(h->bootcmd) - strnlen(h->bootcmd, sizeof(h->bootcmd)) - 1); dev_info(&oct->pci_dev->dev, "Writing boot command: %s\n", h->bootcmd);
In order to remove the usage of strncat(), write directly at the rigth place in the 'h->bootcmd' array and check if the output is truncated. Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> --- The goal is to potentially remove the strncat() function from the kernel. Their are only few users and most of them use it wrongly. This patch is compile tested only. --- .../net/ethernet/cavium/liquidio/octeon_console.c | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-)