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@xxxxxxxxxx> > > --- > > 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@xxxxxxxxxxxxxx/ 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
