Re: linux-next - bnxt buffer overflow in strnlen

[Niklas Cassel]

On Fri, Jan 13, 2023 at 02:44:32PM -0800, Kees Cook wrote:
> On Fri, Jan 13, 2023 at 04:08:21PM +0000, Niklas Cassel wrote:
> > > Hello Kees,
> > > 
> > > Unfortunately, this commit introduces a crash in the bnxt
> > > ethernet driver when booting linux-next.

(snip)

> 
> Let's see...
> 
> struct hwrm_selftest_qlist_output {
> 	...
>         char    test0_name[32];
>         char    test1_name[32];
>         char    test2_name[32];
>         char    test3_name[32];
>         char    test4_name[32];
>         char    test5_name[32];
>         char    test6_name[32];
>         char    test7_name[32];
> 	...
> };
> 
> Ew. So, yes, it's specifically reach past the end of the test0_name[]
> array, *and* is may overflow the heap. Does this patch solve it for you?

Yes, it does!

Thank you very much Kees, both for this patch, and for all the excellent work
that you've done with regard to kernel hardening in general over the years.

Feel free to add my:
Tested-by: Niklas Cassel <niklas.cassel@xxxxxxx>

if you send out a real patch.


Kind regards,
Niklas

> 
> 
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
> index cbf17fcfb7ab..ec573127b707 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
> @@ -3969,7 +3969,7 @@ void bnxt_ethtool_init(struct bnxt *bp)
>  		test_info->timeout = HWRM_CMD_TIMEOUT;
>  	for (i = 0; i < bp->num_tests; i++) {
>  		char *str = test_info->string[i];
> -		char *fw_str = resp->test0_name + i * 32;
> +		char *fw_str = resp->test_name[i];
>  
>  		if (i == BNXT_MACLPBK_TEST_IDX) {
>  			strcpy(str, "Mac loopback test (offline)");
> @@ -3980,14 +3980,9 @@ void bnxt_ethtool_init(struct bnxt *bp)
>  		} else if (i == BNXT_IRQ_TEST_IDX) {
>  			strcpy(str, "Interrupt_test (offline)");
>  		} else {
> -			strscpy(str, fw_str, ETH_GSTRING_LEN);
> -			strncat(str, " test", ETH_GSTRING_LEN - strlen(str));
> -			if (test_info->offline_mask & (1 << i))
> -				strncat(str, " (offline)",
> -					ETH_GSTRING_LEN - strlen(str));
> -			else
> -				strncat(str, " (online)",
> -					ETH_GSTRING_LEN - strlen(str));
> +			snprintf(str, ETH_GSTRING_LEN, "%s test (%s)",
> +				 fw_str, test_info->offline_mask & (1 << i) ?
> +					"offline" : "online");
>  		}
>  	}
>  
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_hsi.h b/drivers/net/ethernet/broadcom/bnxt/bnxt_hsi.h
> index 2686a714a59f..a5408879e077 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_hsi.h
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_hsi.h
> @@ -10249,14 +10249,7 @@ struct hwrm_selftest_qlist_output {
>  	u8	unused_0;
>  	__le16	test_timeout;
>  	u8	unused_1[2];
> -	char	test0_name[32];
> -	char	test1_name[32];
> -	char	test2_name[32];
> -	char	test3_name[32];
> -	char	test4_name[32];
> -	char	test5_name[32];
> -	char	test6_name[32];
> -	char	test7_name[32];
> +	char	test_name[8][32];
>  	u8	eyescope_target_BER_support;
>  	#define SELFTEST_QLIST_RESP_EYESCOPE_TARGET_BER_SUPPORT_BER_1E8_SUPPORTED  0x0UL
>  	#define SELFTEST_QLIST_RESP_EYESCOPE_TARGET_BER_SUPPORT_BER_1E9_SUPPORTED  0x1UL
> 
> 
> 
> 
> 
> -- 
> Kees Cook