| 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
| ||
|
Message-ID: <CALGdzuq5D9=HFBwVDxpJm2MULo-Q4qkQuUfZmEHBrpnNJpefXw@mail.gmail.com>
Date: Sat, 26 Jul 2025 13:21:00 -0700
From: Chenyuan Yang <chenyuan0y@...il.com>
To: Paolo Abeni <pabeni@...hat.com>
Cc: Geethasowjanya Akula <gakula@...vell.com>, Sunil Kovvuri Goutham <sgoutham@...vell.com>,
Subbaraya Sundeep Bhatta <sbhatta@...vell.com>, Hariprasad Kelam <hkelam@...vell.com>,
Bharat Bhushan <bbhushan2@...vell.com>, "andrew+netdev@...n.ch" <andrew+netdev@...n.ch>,
"davem@...emloft.net" <davem@...emloft.net>, "edumazet@...gle.com" <edumazet@...gle.com>,
"kuba@...nel.org" <kuba@...nel.org>, "ast@...nel.org" <ast@...nel.org>,
"daniel@...earbox.net" <daniel@...earbox.net>, "hawk@...nel.org" <hawk@...nel.org>,
"john.fastabend@...il.com" <john.fastabend@...il.com>, "sdf@...ichev.me" <sdf@...ichev.me>,
Suman Ghosh <sumang@...vell.com>, "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
"bpf@...r.kernel.org" <bpf@...r.kernel.org>, "zzjas98@...il.com" <zzjas98@...il.com>
Subject: Re: [EXTERNAL] [PATCH] net: otx2: handle NULL returned by xdp_convert_buff_to_frame()
On Thu, Jul 24, 2025 at 3:11 AM Paolo Abeni <pabeni@...hat.com> wrote:
>
> On 7/23/25 5:36 AM, Geethasowjanya Akula wrote:
> >> -----Original Message-----
> >> From: Geethasowjanya Akula
> >> Sent: Wednesday, July 23, 2025 8:59 AM
> >> To: Chenyuan Yang <chenyuan0y@...il.com>; Sunil Kovvuri Goutham
> >> <sgoutham@...vell.com>; Subbaraya Sundeep Bhatta
> >> <sbhatta@...vell.com>; Hariprasad Kelam <hkelam@...vell.com>; Bharat
> >> Bhushan <bbhushan2@...vell.com>; andrew+netdev@...n.ch;
> >> davem@...emloft.net; edumazet@...gle.com; kuba@...nel.org;
> >> pabeni@...hat.com; ast@...nel.org; daniel@...earbox.net;
> >> hawk@...nel.org; john.fastabend@...il.com; sdf@...ichev.me; Suman
> >> Ghosh <sumang@...vell.com>
> >> Cc: netdev@...r.kernel.org; bpf@...r.kernel.org; zzjas98@...il.com
> >> Subject: RE: [EXTERNAL] [PATCH] net: otx2: handle NULL returned by
> >> xdp_convert_buff_to_frame()
> >>
> >>
> >>
> >>> -----Original Message-----
> >>> From: Chenyuan Yang <chenyuan0y@...il.com>
> >>> Sent: Wednesday, July 23, 2025 6:03 AM
> >>> To: Sunil Kovvuri Goutham <sgoutham@...vell.com>; Geethasowjanya
> >> Akula
> >>> <gakula@...vell.com>; Subbaraya Sundeep Bhatta <sbhatta@...vell.com>;
> >>> Hariprasad Kelam <hkelam@...vell.com>; Bharat Bhushan
> >>> <bbhushan2@...vell.com>; andrew+netdev@...n.ch;
> >> davem@...emloft.net;
> >>> edumazet@...gle.com; kuba@...nel.org; pabeni@...hat.com;
> >>> ast@...nel.org; daniel@...earbox.net; hawk@...nel.org;
> >>> john.fastabend@...il.com; sdf@...ichev.me; Suman Ghosh
> >>> <sumang@...vell.com>
> >>> Cc: netdev@...r.kernel.org; bpf@...r.kernel.org; zzjas98@...il.com;
> >>> Chenyuan Yang <chenyuan0y@...il.com>
> >>> Subject: [EXTERNAL] [PATCH] net: otx2: handle NULL returned by
> >>> xdp_convert_buff_to_frame()
> >>>
> >>> The xdp_convert_buff_to_frame() function can return NULL when there is
> >>> insufficient headroom in the buffer to store the xdp_frame structure or
> >>> when the driver didn't reserve enough tailroom for skb_shared_info.
> >>>
> >>> Currently, the otx2 driver does not check for this NULL return value in
> >>> two critical paths within otx2_xdp_rcv_pkt_handler():
> >>>
> >>> 1. XDP_TX case: Passes potentially NULL xdpf to otx2_xdp_sq_append_pkt()
> >> 2.
> >>> XDP_REDIRECT error path: Calls xdp_return_frame() with potentially NULL
> >>>
> >>> This can lead to kernel crashes due to NULL pointer dereference.
> >>>
> >>> Fix by adding proper NULL checks in both paths. For XDP_TX, return
> >>> false to indicate packet should be dropped. For XDP_REDIRECT error
> >>> path, only call
> >>> xdp_return_frame() if conversion succeeded, otherwise manually free the
> >>> page.
> >>>
> >>> Please correct me if any error path is incorrect.
> >>>
> >>> This is similar to the commit cc3628dcd851
> >>> ("xen-netfront: handle NULL returned by xdp_convert_buff_to_frame()").
> >>>
> >>> Signed-off-by: Chenyuan Yang <chenyuan0y@...il.com>
> >>> Fixes: 94c80f748873 ("octeontx2-pf: use xdp_return_frame() to free xdp
> >>> buffers")
> >>> ---
> >>> drivers/net/ethernet/marvell/octeontx2/nic/otx2_txrx.c | 8 +++++++-
> >>> 1 file changed, 7 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_txrx.c
> >>> b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_txrx.c
> >>> index 99ace381cc78..0c4c050b174a 100644
> >>> --- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_txrx.c
> >>> +++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_txrx.c
> >>> @@ -1534,6 +1534,9 @@ static bool otx2_xdp_rcv_pkt_handler(struct
> >>> otx2_nic *pfvf,
> >>> qidx += pfvf->hw.tx_queues;
> >>> cq->pool_ptrs++;
> >>> xdpf = xdp_convert_buff_to_frame(&xdp);
> >>> + if (unlikely(!xdpf))
> >>> + return false;
> >>> +
> >>> return otx2_xdp_sq_append_pkt(pfvf, xdpf,
> >>> cqe->sg.seg_addr,
> >>> cqe->sg.seg_size,
> >>> @@ -1558,7 +1561,10 @@ static bool otx2_xdp_rcv_pkt_handler(struct
> >>> otx2_nic *pfvf,
> >>> otx2_dma_unmap_page(pfvf, iova, pfvf->rbsize,
> >>> DMA_FROM_DEVICE);
> >>> xdpf = xdp_convert_buff_to_frame(&xdp);
> >>> - xdp_return_frame(xdpf);
> >>> + if (likely(xdpf))
> >>> + xdp_return_frame(xdpf);
> >>> + else
> >>> + put_page(page);
> >> Thanks for the fix. Given that the page is already freed, returning true in this
> >> case makes sense.
> > This change might not be directly related to the current patch, though. You can either
> > include it here or we can submit a follow-up patch to address it.
>
> If I read correctly, returning false as the current patch is doing, will
> make the later code in otx2_rcv_pkt_handler() unconditionally use the
> just freed page.
>
> I think returning true after put_page() is strictly necessary.
Thanks for the review and for catching that issue. You're right,
returning false would cause a use-after-free, as the caller would
proceed to use the already freed page.
I've updated the patch to return true in the XDP_TX failure case. I
also adjusted the XDP_REDIRECT error path to do the same after calling
put_page(), preventing a fall-through.
Does the updated patch below look correct? If so, I'll send out a formal v2.
---
drivers/net/ethernet/marvell/octeontx2/nic/otx2_txrx.c | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_txrx.c
b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_txrx.c
index 99ace381cc78..4e1b9a3f6e51 100644
--- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_txrx.c
+++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_txrx.c
@@ -1534,6 +1534,9 @@ static bool otx2_xdp_rcv_pkt_handler(struct
otx2_nic *pfvf,
qidx += pfvf->hw.tx_queues;
cq->pool_ptrs++;
xdpf = xdp_convert_buff_to_frame(&xdp);
+ if (unlikely(!xdpf))
+ return true;
+
return otx2_xdp_sq_append_pkt(pfvf, xdpf,
cqe->sg.seg_addr,
cqe->sg.seg_size,
@@ -1558,7 +1561,12 @@ static bool otx2_xdp_rcv_pkt_handler(struct
otx2_nic *pfvf,
otx2_dma_unmap_page(pfvf, iova, pfvf->rbsize,
DMA_FROM_DEVICE);
xdpf = xdp_convert_buff_to_frame(&xdp);
- xdp_return_frame(xdpf);
+ if (likely(xdpf)) {
+ xdp_return_frame(xdpf);
+ } else {
+ put_page(page);
+ return true;
+ }
break;
default:
bpf_warn_invalid_xdp_action(pfvf->netdev, prog, act);
---
> /P
>
Powered by blists - more mailing lists