[PATCH net 0/2] liquidio: Fix an off by one in octeon_download_firmware()

[PATCH net 0/2] liquidio: Fix an off by one in octeon_download_firmware()

[Christophe JAILLET]

This serie fixes an off by one related to the usage of strncat() in
octeon_download_firmware().

The first patch is a minimal fix.

The 2nd one is an attempt to remove strncat() which is used in a wrong way
most of the time.
It removes the need of an intermediate buffer but may need further discussions.
(i.e. is it a good idea to update h->bootcmd directly?)

Both patches are compile tested only.

Christophe JAILLET (2):
  liquidio: Fix an off by one in octeon_download_firmware()
  liquidio: Simplify octeon_download_firmware()

 .../net/ethernet/cavium/liquidio/octeon_console.c   | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

-- 
2.34.1

[PATCH net 1/2] liquidio: Fix an off by one in octeon_download_firmware()

[Christophe JAILLET]

In order to append the 'boottime' string to 'h->bootcmd', the final NULL
has to betaken into account when checking if there is enough space.

Fixes: 907aaa6babe1 ("liquidio: pass date and time info to NIC firmware")
Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
 drivers/net/ethernet/cavium/liquidio/octeon_console.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/cavium/liquidio/octeon_console.c b/drivers/net/ethernet/cavium/liquidio/octeon_console.c
index 67c3570f875f..bd6baf2872a5 100644
--- a/drivers/net/ethernet/cavium/liquidio/octeon_console.c
+++ b/drivers/net/ethernet/cavium/liquidio/octeon_console.c
@@ -899,13 +899,13 @@ 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))) <
+	if ((sizeof(h->bootcmd) - strnlen(h->bootcmd, sizeof(h->bootcmd))) <=
 		ret) {
 		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)));
+		sizeof(h->bootcmd) - strnlen(h->bootcmd, sizeof(h->bootcmd)) - 1);
 
 	dev_info(&oct->pci_dev->dev, "Writing boot command: %s\n",
 		 h->bootcmd);
-- 
2.34.1

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

[Christophe JAILLET]

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);
-	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);
-- 
2.34.1

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

[Jacob Keller]

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

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

[Paolo Abeni]

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

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@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