netdev - Re: [PATCH net-next] eth: fbnic: fix ubsan complaints about OOB accesses

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]

Message-ID: <bfb5122f-e603-457c-8115-4c4acfe7360b@intel.com>
Date: Wed, 9 Jul 2025 14:23:11 -0700
From: Jacob Keller <jacob.e.keller@...el.com>
To: Jakub Kicinski <kuba@...nel.org>, <davem@...emloft.net>
CC: <netdev@...r.kernel.org>, <edumazet@...gle.com>, <pabeni@...hat.com>,
	<andrew+netdev@...n.ch>, <horms@...nel.org>, Alexander Duyck
	<alexanderduyck@...com>, <lee@...ger.us>
Subject: Re: [PATCH net-next] eth: fbnic: fix ubsan complaints about OOB
 accesses



On 7/9/2025 1:59 PM, Jakub Kicinski wrote:
> UBSAN complains that we reach beyond the end of the log entry:
> 
>    UBSAN: array-index-out-of-bounds in drivers/net/ethernet/meta/fbnic/fbnic_fw_log.c:94:50
>    index 71 is out of range for type 'char [*]'
>    Call Trace:
>     <TASK>
>     ubsan_epilogue+0x5/0x2b
>     fbnic_fw_log_write+0x120/0x960
>     fbnic_fw_parse_logs+0x161/0x210
> 
> We're just taking the address of the character after the array,
> so this really seems like something that should be legal.
> But whatever, easy enough to silence by doing direct pointer math.
> 

My guess is because you're pointing the next entry at a pointer
generated from the address of a flexible array marked with __counted_by.
UBSAN assumes the array value ends at the end of the flexible array.. it
has no context that this is really a larger buffer.

> Fixes: c2b93d6beca8 ("eth: fbnic: Create ring buffer for firmware logs")
> Reviewed-by: Alexander Duyck <alexanderduyck@...com>
> Signed-off-by: Jakub Kicinski <kuba@...nel.org>
> ---
> CC: alexanderduyck@...com
> CC: jacob.e.keller@...el.com
> CC: lee@...ger.us
> ---
>  drivers/net/ethernet/meta/fbnic/fbnic_fw_log.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_fw_log.c b/drivers/net/ethernet/meta/fbnic/fbnic_fw_log.c
> index 38749d47cee6..c1663f042245 100644
> --- a/drivers/net/ethernet/meta/fbnic/fbnic_fw_log.c
> +++ b/drivers/net/ethernet/meta/fbnic/fbnic_fw_log.c
> @@ -91,16 +91,16 @@ int fbnic_fw_log_write(struct fbnic_dev *fbd, u64 index, u32 timestamp,
>  		entry = log->data_start;
>  	} else {
>  		head = list_first_entry(&log->entries, typeof(*head), list);
> -		entry = (struct fbnic_fw_log_entry *)&head->msg[head->len + 1]; 

I am guessing that UBSAN gets info about the hint for the length of the
msg, via the counted_by annotation in the structure? Then it realizes
that this is too large. Strictly taking address of a value doesn't
actually directly access the memory... However, you then later access
the value via the entry variable.. Perhaps UBSAN is complaining about that?

> -		entry = PTR_ALIGN(entry, 8);
> +		entry_end = head->msg + head->len + 1;
> +		entry = PTR_ALIGN(entry_end, 8);
>  	}

Regardless, this replacement looks correct. And if it makes the UBSAN
happy, thats good.

Reviewed-by: Jacob Keller <jacob.e.keller@...el.com>

>  
> -	entry_end = &entry->msg[msg_len + 1];
> +	entry_end = entry->msg + msg_len + 1;
>  
>  	/* We've reached the end of the buffer, wrap around */
>  	if (entry_end > log->data_end) {
>  		entry = log->data_start;
> -		entry_end = &entry->msg[msg_len + 1];
> +		entry_end = entry->msg + msg_len + 1;
>  	}
>  
>  	/* Make room for entry by removing from tail. */




Download attachment "OpenPGP_signature.asc" of type "application/pgp-signature" (237 bytes)