[PATCH net] nfp: bpf: prevent integer overflow in nfp_bpf_event_output()

[PATCH net] nfp: bpf: prevent integer overflow in nfp_bpf_event_output()

[Dan Carpenter]

The "sizeof(struct cmsg_bpf_event) + pkt_size + data_size" math could
potentially have an integer wrapping bug on 32bit systems.  Check for
this and return an error.

Fixes: 9816dd35ecec ("nfp: bpf: perf event output helpers support")
Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
---
 drivers/net/ethernet/netronome/nfp/bpf/offload.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/netronome/nfp/bpf/offload.c b/drivers/net/ethernet/netronome/nfp/bpf/offload.c
index 9d97cd281f18..c03558adda91 100644
--- a/drivers/net/ethernet/netronome/nfp/bpf/offload.c
+++ b/drivers/net/ethernet/netronome/nfp/bpf/offload.c
@@ -458,7 +458,8 @@ int nfp_bpf_event_output(struct nfp_app_bpf *bpf, const void *data,
 	map_id_full = be64_to_cpu(cbe->map_ptr);
 	map_id = map_id_full;
 
-	if (len < sizeof(struct cmsg_bpf_event) + pkt_size + data_size)
+	if (size_add(pkt_size, data_size) > INT_MAX ||
+	    len < sizeof(struct cmsg_bpf_event) + pkt_size + data_size)
 		return -EINVAL;
 	if (cbe->hdr.ver != NFP_CCM_ABI_VERSION)
 		return -EINVAL;
-- 
2.45.2

Re: [PATCH net] nfp: bpf: prevent integer overflow in nfp_bpf_event_output()

[Alexander Lobakin]

From: Dan Carpenter <dan.carpenter@linaro.org>
Date: Mon, 13 Jan 2025 09:18:39 +0300

> The "sizeof(struct cmsg_bpf_event) + pkt_size + data_size" math could
> potentially have an integer wrapping bug on 32bit systems.  Check for

Not in practice I suppose? Do we need to fix "never" bugs?

> this and return an error.
> 
> Fixes: 9816dd35ecec ("nfp: bpf: perf event output helpers support")
> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>

Thanks,
Olek

Re: [PATCH net] nfp: bpf: prevent integer overflow in nfp_bpf_event_output()

[Dan Carpenter]

[ I tried to send this email yesterday but apparently gmail blocked
  it for security reasons?  So weird. - dan ]

On Mon, Jan 13, 2025 at 01:32:11PM +0100, Alexander Lobakin wrote:
> From: Dan Carpenter <dan.carpenter@linaro.org>
> Date: Mon, 13 Jan 2025 09:18:39 +0300
> 
> > The "sizeof(struct cmsg_bpf_event) + pkt_size + data_size" math could
> > potentially have an integer wrapping bug on 32bit systems.  Check for
> 
> Not in practice I suppose? Do we need to fix "never" bugs?
> 

No, this is from static analysis.  We don't need to fix never bugs.

This is called from nfp_bpf_ctrl_msg_rx() and nfp_bpf_ctrl_msg_rx_raw()
and I assumed that since pkt_size and data_size come from skb->data on
the rx path then they couldn't be trusted.

Where is the bounds checking?

regards,
dan carpenter

Re: [PATCH net] nfp: bpf: prevent integer overflow in nfp_bpf_event_output()

[Alexander Lobakin]

From: Dan Carpenter <dan.carpenter@linaro.org>
Date: Tue, 14 Jan 2025 13:45:04 +0300

> [ I tried to send this email yesterday but apparently gmail blocked
>   it for security reasons?  So weird. - dan ]
> 
> On Mon, Jan 13, 2025 at 01:32:11PM +0100, Alexander Lobakin wrote:
>> From: Dan Carpenter <dan.carpenter@linaro.org>
>> Date: Mon, 13 Jan 2025 09:18:39 +0300
>>
>>> The "sizeof(struct cmsg_bpf_event) + pkt_size + data_size" math could
>>> potentially have an integer wrapping bug on 32bit systems.  Check for
>>
>> Not in practice I suppose? Do we need to fix "never" bugs?
>>
> 
> No, this is from static analysis.  We don't need to fix never bugs.
> 
> This is called from nfp_bpf_ctrl_msg_rx() and nfp_bpf_ctrl_msg_rx_raw()
> and I assumed that since pkt_size and data_size come from skb->data on
> the rx path then they couldn't be trusted.

skbs are always valid and skb->len could never cross INT_MAX to provoke
an overflow.

> 
> Where is the bounds checking?
> 
> regards,
> dan carpenter

Thanks,
Olek

Re: [PATCH net] nfp: bpf: prevent integer overflow in nfp_bpf_event_output()

[Dan Carpenter]

On Tue, Jan 14, 2025 at 06:17:22PM +0100, Alexander Lobakin wrote:
> From: Dan Carpenter <dan.carpenter@linaro.org>
> Date: Tue, 14 Jan 2025 13:45:04 +0300
> 
> > [ I tried to send this email yesterday but apparently gmail blocked
> >   it for security reasons?  So weird. - dan ]
> > 
> > On Mon, Jan 13, 2025 at 01:32:11PM +0100, Alexander Lobakin wrote:
> >> From: Dan Carpenter <dan.carpenter@linaro.org>
> >> Date: Mon, 13 Jan 2025 09:18:39 +0300
> >>
> >>> The "sizeof(struct cmsg_bpf_event) + pkt_size + data_size" math could
> >>> potentially have an integer wrapping bug on 32bit systems.  Check for
> >>
> >> Not in practice I suppose? Do we need to fix "never" bugs?
> >>
> > 
> > No, this is from static analysis.  We don't need to fix never bugs.
> > 
> > This is called from nfp_bpf_ctrl_msg_rx() and nfp_bpf_ctrl_msg_rx_raw()
> > and I assumed that since pkt_size and data_size come from skb->data on
> > the rx path then they couldn't be trusted.
> 
> skbs are always valid and skb->len could never cross INT_MAX to provoke
> an overflow.
> 

True but unrelated.  I think you are looking at the wrong code...

drivers/net/ethernet/netronome/nfp/bpf/offload.c
   445  int nfp_bpf_event_output(struct nfp_app_bpf *bpf, const void *data,
                                                                      ^^^^
This code comes from the network so it cannot be trusted.

   446                           unsigned int len)
   447  {
   448          struct cmsg_bpf_event *cbe = (void *)data;
                                       ^^^^^^^^^^^^^^^^^^^
It is cast to a struct here.

   449          struct nfp_bpf_neutral_map *record;
   450          u32 pkt_size, data_size, map_id;
   451          u64 map_id_full;
   452  
   453          if (len < sizeof(struct cmsg_bpf_event))
   454                  return -EINVAL;
   455  
   456          pkt_size = be32_to_cpu(cbe->pkt_size);
   457          data_size = be32_to_cpu(cbe->data_size);

pkt_size and data_size are u32 values which are controlled from
over the network.

   458          map_id_full = be64_to_cpu(cbe->map_ptr);
   459          map_id = map_id_full;
   460  
   461          if (len < sizeof(struct cmsg_bpf_event) + pkt_size + data_size)
                          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
On a 32bit system, then this math can overflow.  The pkt_size and
data_size are too high and we should return -EINVAL but this check
doesn't work because of integer wrapping.

   462                  return -EINVAL;
   63          if (cbe->hdr.ver != NFP_CCM_ABI_VERSION)
   464                  return -EINVAL;
   465  
   466          rcu_read_lock();
   467          record = rhashtable_lookup(&bpf->maps_neutral, &map_id,
   468                                     nfp_bpf_maps_neutral_params);
   469          if (!record || map_id_full > U32_MAX) {
   470                  rcu_read_unlock();
   471                  cmsg_warn(bpf, "perf event: map id %lld (0x%llx) not recognized, dropping event\n",
   472                            map_id_full, map_id_full);
   473                  return -EINVAL;
   474          }
   475  
   476          bpf_event_output(record->ptr, be32_to_cpu(cbe->cpu_id),
   477                           &cbe->data[round_up(pkt_size, 4)], data_size,
                                            ^^^^^^^^^^^^^^^^^^^^^
Here we are way out of bounds with regards to the cbe->data[] array.

regards,
dan carpenter

   478                           cbe->data, pkt_size, nfp_bpf_perf_event_copy);
   479          rcu_read_unlock();
   480  
   481          return 0;
   482  }

Re: [PATCH net] nfp: bpf: prevent integer overflow in nfp_bpf_event_output()

[Alexander Lobakin]

From: Dan Carpenter <dan.carpenter@linaro.org>
Date: Tue, 14 Jan 2025 21:43:26 +0300

> On Tue, Jan 14, 2025 at 06:17:22PM +0100, Alexander Lobakin wrote:
>> From: Dan Carpenter <dan.carpenter@linaro.org>
>> Date: Tue, 14 Jan 2025 13:45:04 +0300
>>
>>> [ I tried to send this email yesterday but apparently gmail blocked
>>>   it for security reasons?  So weird. - dan ]
>>>
>>> On Mon, Jan 13, 2025 at 01:32:11PM +0100, Alexander Lobakin wrote:
>>>> From: Dan Carpenter <dan.carpenter@linaro.org>
>>>> Date: Mon, 13 Jan 2025 09:18:39 +0300
>>>>
>>>>> The "sizeof(struct cmsg_bpf_event) + pkt_size + data_size" math could
>>>>> potentially have an integer wrapping bug on 32bit systems.  Check for
>>>>
>>>> Not in practice I suppose? Do we need to fix "never" bugs?
>>>>
>>>
>>> No, this is from static analysis.  We don't need to fix never bugs.
>>>
>>> This is called from nfp_bpf_ctrl_msg_rx() and nfp_bpf_ctrl_msg_rx_raw()
>>> and I assumed that since pkt_size and data_size come from skb->data on
>>> the rx path then they couldn't be trusted.
>>
>> skbs are always valid and skb->len could never cross INT_MAX to provoke
>> an overflow.
>>
> 
> True but unrelated.  I think you are looking at the wrong code...
> 
> drivers/net/ethernet/netronome/nfp/bpf/offload.c
>    445  int nfp_bpf_event_output(struct nfp_app_bpf *bpf, const void *data,
>                                                                       ^^^^
> This code comes from the network so it cannot be trusted.
> 
>    446                           unsigned int len)
>    447  {
>    448          struct cmsg_bpf_event *cbe = (void *)data;
>                                        ^^^^^^^^^^^^^^^^^^^
> It is cast to a struct here.
> 
>    449          struct nfp_bpf_neutral_map *record;
>    450          u32 pkt_size, data_size, map_id;
>    451          u64 map_id_full;
>    452  
>    453          if (len < sizeof(struct cmsg_bpf_event))
>    454                  return -EINVAL;
>    455  
>    456          pkt_size = be32_to_cpu(cbe->pkt_size);
>    457          data_size = be32_to_cpu(cbe->data_size);
> 
> pkt_size and data_size are u32 values which are controlled from
> over the network.
> 
>    458          map_id_full = be64_to_cpu(cbe->map_ptr);
>    459          map_id = map_id_full;
>    460  
>    461          if (len < sizeof(struct cmsg_bpf_event) + pkt_size + data_size)
>                           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> On a 32bit system, then this math can overflow.  The pkt_size and
> data_size are too high and we should return -EINVAL but this check
> doesn't work because of integer wrapping.
> 
>    462                  return -EINVAL;
>    63          if (cbe->hdr.ver != NFP_CCM_ABI_VERSION)
>    464                  return -EINVAL;
>    465  
>    466          rcu_read_lock();
>    467          record = rhashtable_lookup(&bpf->maps_neutral, &map_id,
>    468                                     nfp_bpf_maps_neutral_params);
>    469          if (!record || map_id_full > U32_MAX) {
>    470                  rcu_read_unlock();
>    471                  cmsg_warn(bpf, "perf event: map id %lld (0x%llx) not recognized, dropping event\n",
>    472                            map_id_full, map_id_full);
>    473                  return -EINVAL;
>    474          }
>    475  
>    476          bpf_event_output(record->ptr, be32_to_cpu(cbe->cpu_id),
>    477                           &cbe->data[round_up(pkt_size, 4)], data_size,
>                                             ^^^^^^^^^^^^^^^^^^^^^
> Here we are way out of bounds with regards to the cbe->data[] array.

Aaah okay. Thanks for the detailed explanation! My note was "generic",
not regarding precisely this case.

I think a couple checks here wouldn't hurt, not only for an overflow,
but functional ones whether we have sane values. It's an ask more for
the Netronome guys tho.

Regarding your particular patch, I think it's now pretty clear for me:

Reviewed-by: Alexander Lobakin <aleksander.lobakin@intel.com>

> 
> regards,
> dan carpenter

Thanks,
Olek

Re: [PATCH net] nfp: bpf: prevent integer overflow in nfp_bpf_event_output()

[patchwork-bot+netdevbpf]

Hello:

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

On Mon, 13 Jan 2025 09:18:39 +0300 you wrote:
> The "sizeof(struct cmsg_bpf_event) + pkt_size + data_size" math could
> potentially have an integer wrapping bug on 32bit systems.  Check for
> this and return an error.
> 
> Fixes: 9816dd35ecec ("nfp: bpf: perf event output helpers support")
> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> 
> [...]

Here is the summary with links:
  - [net] nfp: bpf: prevent integer overflow in nfp_bpf_event_output()
    https://git.kernel.org/netdev/net/c/16ebb6f5b629

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