[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <9a6d37de-14a4-483a-a515-ac30dbea4d4c@kadam.mountain>
Date: Tue, 24 Oct 2023 15:10:50 +0300
From: Dan Carpenter <dan.carpenter@...aro.org>
To: Paolo Abeni <pabeni@...hat.com>
Cc: Christophe JAILLET <christophe.jaillet@...adoo.fr>,
dchickles@...vell.com, sburla@...vell.com, fmanlunas@...vell.com,
davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
veerasenareddy.burru@...ium.com, felix.manlunas@...ium.com,
netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
kernel-janitors@...r.kernel.org
Subject: Re: [PATCH net 2/2] liquidio: Simplify octeon_download_firmware()
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@...adoo.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
Powered by blists - more mailing lists