| 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:
<CH0PR18MB43390F3BFF4500653CB3F14FCD5AA@CH0PR18MB4339.namprd18.prod.outlook.com>
Date: Mon, 28 Jul 2025 08:38:16 +0000
From: Geethasowjanya Akula <gakula@...vell.com>
To: Chenyuan Yang <chenyuan0y@...il.com>, Paolo Abeni <pabeni@...hat.com>
CC: 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()
>-----Original Message-----
>From: Chenyuan Yang <chenyuan0y@...il.com>
>Sent: Sunday, July 27, 2025 1:51 AM
>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;
>davem@...emloft.net; edumazet@...gle.com; kuba@...nel.org;
>ast@...nel.org; daniel@...earbox.net; hawk@...nel.org;
>john.fastabend@...il.com; sdf@...ichev.me; Suman Ghosh
><sumang@...vell.com>; 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()
>
>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
Returning true on XDP_TX failure requires the page to be freed. Otherwise, it should
return false so the packet can be handled by otx2_rcv_pkt_handler.
>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