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

[Jacob Keller <jacob.e.keller@intel.com>]

[-- Attachment #1.1: Type: text/plain, Size: 2848 bytes --]



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@fb.com>
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
> CC: alexanderduyck@fb.com
> CC: jacob.e.keller@intel.com
> CC: lee@trager.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@intel.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. */



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]