Re: [PATCH net 2/2] liquidio: Simplify octeon_download_firmware()

[Dan Carpenter]

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