On Tue, Jan 14, 2025 at 06:17:22PM +0100, Alexander Lobakin wrote:
> From: Dan Carpenter <dan.carpenter@xxxxxxxxxx>
> 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@xxxxxxxxxx>
> >> 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 }
