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

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

[Jakub Kicinski]

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.

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];
-		entry = PTR_ALIGN(entry, 8);
+		entry_end = head->msg + head->len + 1;
+		entry = PTR_ALIGN(entry_end, 8);
 	}
 
-	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. */
-- 
2.50.0

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

[Jacob Keller]

[-- 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 --]

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

[Jakub Kicinski]

On Wed, 9 Jul 2025 14:23:11 -0700 Jacob Keller wrote:
> >  		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?

Could be.. The splat includes the line info for the line whether entry
is computed, but maybe that's just a nicety and the detection is done
at access time..

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

[Jacob Keller]

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



On 7/9/2025 5:11 PM, Jakub Kicinski wrote:
> On Wed, 9 Jul 2025 14:23:11 -0700 Jacob Keller wrote:
>>>  		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?
> 
> Could be.. The splat includes the line info for the line whether entry
> is computed, but maybe that's just a nicety and the detection is done
> at access time..

It might just be the way the pointer is generated. I think I had issues
with something similar when working on lib/pldmfw too... Been a while
since I had an UBSAN splat though.

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

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

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Wed,  9 Jul 2025 13:59:10 -0700 you 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
> 
> [...]

Here is the summary with links:
  - [net-next] eth: fbnic: fix ubsan complaints about OOB accesses
    https://git.kernel.org/netdev/net-next/c/0346000aaab8

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html