netdev - Re: [PATCH net 2/2] liquidio: Simplify octeon_download

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Mon, 23 Oct 2023 17:29:48 -0700
From: Jacob Keller <jacob.e.keller@...el.com>
To: Christophe JAILLET <christophe.jaillet@...adoo.fr>,
	<dchickles@...vell.com>, <sburla@...vell.com>, <fmanlunas@...vell.com>,
	<davem@...emloft.net>, <edumazet@...gle.com>, <kuba@...nel.org>,
	<pabeni@...hat.com>, <veerasenareddy.burru@...ium.com>
CC: <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 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@...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.
> ---
>  .../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