[PATCH] xen-netfront: handle NULL returned by xdp_convert_buff_to_frame()

[PATCH] xen-netfront: handle NULL returned by xdp_convert_buff_to_frame()

[Alexey Nepomnyashih]

The function xdp_convert_buff_to_frame() may return NULL if it fails
to correctly convert the XDP buffer into an XDP frame due to memory
constraints, internal errors, or invalid data. Failing to check for NULL
may lead to a NULL pointer dereference if the result is used later in
processing, potentially causing crashes, data corruption, or undefined
behavior.

Found by Linux Verification Center (linuxtesting.org) with SVACE.

Cc: stable@vger.kernel.org # v5.9+
Fixes: 6c5aa6fc4def ("xen networking: add basic XDP support for xen-netfront")
Signed-off-by: Alexey Nepomnyashih <sdl@nppct.ru>
---
 drivers/net/xen-netfront.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c
index 8425226c09f0..e99561de3cda 100644
--- a/drivers/net/xen-netfront.c
+++ b/drivers/net/xen-netfront.c
@@ -987,6 +987,10 @@ static u32 xennet_run_xdp(struct netfront_queue *queue, struct page *pdata,
 	case XDP_TX:
 		get_page(pdata);
 		xdpf = xdp_convert_buff_to_frame(xdp);
+		if (unlikely(!xdpf)) {
+			trace_xdp_exception(queue->info->netdev, prog, act);
+			break;
+		}
 		err = xennet_xdp_xmit(queue->info->netdev, 1, &xdpf, 0);
 		if (unlikely(!err))
 			xdp_return_frame_rx_napi(xdpf);
-- 
2.43.0

Re: [PATCH] xen-netfront: handle NULL returned by xdp_convert_buff_to_frame()

[Jakub Kicinski]

On Mon, 14 Apr 2025 18:34:01 +0000 Alexey Nepomnyashih wrote:
>  		get_page(pdata);

Please notice this get_page() here.

>  		xdpf = xdp_convert_buff_to_frame(xdp);
> +		if (unlikely(!xdpf)) {
> +			trace_xdp_exception(queue->info->netdev, prog, act);
> +			break;
> +		}

Re: [PATCH] xen-netfront: handle NULL returned by xdp_convert_buff_to_frame()

[Alexey]

On 17.04.2025 03:58, Jakub Kicinski wrote:
> On Mon, 14 Apr 2025 18:34:01 +0000 Alexey Nepomnyashih wrote:
>>   		get_page(pdata);
> Please notice this get_page() here.
>
>>   		xdpf = xdp_convert_buff_to_frame(xdp);
>> +		if (unlikely(!xdpf)) {
>> +			trace_xdp_exception(queue->info->netdev, prog, act);
>> +			break;
>> +		}
Do you mean that it would be better to move the get_page(pdata) call lower,
after checking for NULL in xdpf, so that the reference count is only 
increased
after a successful conversion?

Re: [PATCH] xen-netfront: handle NULL returned by xdp_convert_buff_to_frame()

[Jürgen Groß]

[-- Attachment #1.1.1: Type: text/plain, Size: 966 bytes --]

On 17.04.25 09:00, Alexey wrote:
> 
> On 17.04.2025 03:58, Jakub Kicinski wrote:
>> On Mon, 14 Apr 2025 18:34:01 +0000 Alexey Nepomnyashih wrote:
>>>           get_page(pdata);
>> Please notice this get_page() here.
>>
>>>           xdpf = xdp_convert_buff_to_frame(xdp);
>>> +        if (unlikely(!xdpf)) {
>>> +            trace_xdp_exception(queue->info->netdev, prog, act);
>>> +            break;
>>> +        }
> Do you mean that it would be better to move the get_page(pdata) call lower,
> after checking for NULL in xdpf, so that the reference count is only increased
> after a successful conversion?

I think the error handling here is generally broken (or at least very
questionable).

I suspect that in case of at least some errors the get_page() is leaking
even without this new patch.

In case I'm wrong a comment reasoning why there is no leak should be
added.


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3743 bytes --]

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

Re: [PATCH] xen-netfront: handle NULL returned by xdp_convert_buff_to_frame()

[Alexey]

On 17.04.2025 10:12, Jürgen Groß wrote:
> On 17.04.25 09:00, Alexey wrote:
>>
>> On 17.04.2025 03:58, Jakub Kicinski wrote:
>>> On Mon, 14 Apr 2025 18:34:01 +0000 Alexey Nepomnyashih wrote:
>>>>           get_page(pdata);
>>> Please notice this get_page() here.
>>>
>>>>           xdpf = xdp_convert_buff_to_frame(xdp);
>>>> +        if (unlikely(!xdpf)) {
>>>> +            trace_xdp_exception(queue->info->netdev, prog, act);
>>>> +            break;
>>>> +        }
>> Do you mean that it would be better to move the get_page(pdata) call 
>> lower,
>> after checking for NULL in xdpf, so that the reference count is only 
>> increased
>> after a successful conversion?
>
> I think the error handling here is generally broken (or at least very
> questionable).
>
> I suspect that in case of at least some errors the get_page() is leaking
> even without this new patch.
>
> In case I'm wrong a comment reasoning why there is no leak should be
> added.
>
>
> Juergen

I think pdata is freed in xdp_return_frame_rx_napi() -> __xdp_return()

Re: [PATCH] xen-netfront: handle NULL returned by xdp_convert_buff_to_frame()

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 1324 bytes --]

On 17.04.25 10:45, Alexey wrote:
> 
> On 17.04.2025 10:12, Jürgen Groß wrote:
>> On 17.04.25 09:00, Alexey wrote:
>>>
>>> On 17.04.2025 03:58, Jakub Kicinski wrote:
>>>> On Mon, 14 Apr 2025 18:34:01 +0000 Alexey Nepomnyashih wrote:
>>>>>           get_page(pdata);
>>>> Please notice this get_page() here.
>>>>
>>>>>           xdpf = xdp_convert_buff_to_frame(xdp);
>>>>> +        if (unlikely(!xdpf)) {
>>>>> +            trace_xdp_exception(queue->info->netdev, prog, act);
>>>>> +            break;
>>>>> +        }
>>> Do you mean that it would be better to move the get_page(pdata) call lower,
>>> after checking for NULL in xdpf, so that the reference count is only increased
>>> after a successful conversion?
>>
>> I think the error handling here is generally broken (or at least very
>> questionable).
>>
>> I suspect that in case of at least some errors the get_page() is leaking
>> even without this new patch.
>>
>> In case I'm wrong a comment reasoning why there is no leak should be
>> added.
>>
>>
>> Juergen
> 
> I think pdata is freed in xdp_return_frame_rx_napi() -> __xdp_return()

Agreed. But what if xennet_xdp_xmit() returns an error < 0?

In this case xdp_return_frame_rx_napi() won't be called.


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3743 bytes --]

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

Re: [PATCH] xen-netfront: handle NULL returned by xdp_convert_buff_to_frame()

[Jürgen Groß]

[-- Attachment #1.1.1: Type: text/plain, Size: 2865 bytes --]

On 17.04.25 12:06, Alexey wrote:
> 
> On 17.04.2025 11:51, Juergen Gross wrote:
>> On 17.04.25 10:45, Alexey wrote:
>>>
>>> On 17.04.2025 10:12, Jürgen Groß wrote:
>>>> On 17.04.25 09:00, Alexey wrote:
>>>>>
>>>>> On 17.04.2025 03:58, Jakub Kicinski wrote:
>>>>>> On Mon, 14 Apr 2025 18:34:01 +0000 Alexey Nepomnyashih wrote:
>>>>>>>           get_page(pdata);
>>>>>> Please notice this get_page() here.
>>>>>>
>>>>>>>           xdpf = xdp_convert_buff_to_frame(xdp);
>>>>>>> +        if (unlikely(!xdpf)) {
>>>>>>> + trace_xdp_exception(queue->info->netdev, prog, act);
>>>>>>> +            break;
>>>>>>> +        }
>>>>> Do you mean that it would be better to move the get_page(pdata) call lower,
>>>>> after checking for NULL in xdpf, so that the reference count is only increased
>>>>> after a successful conversion?
>>>>
>>>> I think the error handling here is generally broken (or at least very
>>>> questionable).
>>>>
>>>> I suspect that in case of at least some errors the get_page() is leaking
>>>> even without this new patch.
>>>>
>>>> In case I'm wrong a comment reasoning why there is no leak should be
>>>> added.
>>>>
>>>>
>>>> Juergen
>>>
>>> I think pdata is freed in xdp_return_frame_rx_napi() -> __xdp_return()
>>
>> Agreed. But what if xennet_xdp_xmit() returns an error < 0?
>>
>> In this case xdp_return_frame_rx_napi() won't be called.
>>
>>
>> Juergen
> 
> Agreed. There is no explicit freed pdata in the calling function
> xennet_get_responses(). Without this, the page referenced by pdata
> could be leaked.
> 
> I suggest:
 >
 >  	case XDP_TX:
 > -		get_page(pdata);
 >  		xdpf = xdp_convert_buff_to_frame(xdp);
 > +		if (unlikely(!xdpf)) {
 > +			trace_xdp_exception(queue->info->netdev, prog, act);
 > +			break;
 > +		}
 > +		get_page(pdata);
 >  		err = xennet_xdp_xmit(queue->info->netdev, 1, &xdpf, 0);
 >  		if (unlikely(!err))
 >  			xdp_return_frame_rx_napi(xdpf);
 > -		else if (unlikely(err < 0))
 > +		else if (unlikely(err < 0)) {
 >  			trace_xdp_exception(queue->info->netdev, prog, act);
 > +			xdp_return_frame_rx_napi(xdpf);
 > +		}

Could you please merge the two if () blocks, as they share the
call of xdp_return_frame_rx_napi() now? Something like:

if (unlikely(err <= 0)) {
	if (err < 0)
		trace_xdp_exception(queue->info->netdev, prog, act);
	xdp_return_frame_rx_napi(xdpf);
}

 >  		break;
 >  	case XDP_REDIRECT:
 >  		get_page(pdata);
 >  		err = xdp_do_redirect(queue->info->netdev, xdp, prog);
 >  		*need_xdp_flush = true;
 > -		if (unlikely(err))
 > +		if (unlikely(err)) {
 >  			trace_xdp_exception(queue->info->netdev, prog, act);
 > +			__xdp_return(page_address(pdata), &xdp->mem, true, xdp);
 > +		}
 >  		break;


Juergen

P.S.: please don't use HTML in emails

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3743 bytes --]

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

Re: [PATCH] xen-netfront: handle NULL returned by xdp_convert_buff_to_frame()

[Alexey]

On 17.04.2025 13:23, Jürgen Groß wrote:
> On 17.04.25 12:06, Alexey wrote:
>>
>> On 17.04.2025 11:51, Juergen Gross wrote:
>>> On 17.04.25 10:45, Alexey wrote:
>>>>
>>>> On 17.04.2025 10:12, Jürgen Groß wrote:
>>>>> On 17.04.25 09:00, Alexey wrote:
>>>>>>
>>>>>> On 17.04.2025 03:58, Jakub Kicinski wrote:
>>>>>>> On Mon, 14 Apr 2025 18:34:01 +0000 Alexey Nepomnyashih wrote:
>>>>>>>>           get_page(pdata);
>>>>>>> Please notice this get_page() here.
>>>>>>>
>>>>>>>>           xdpf = xdp_convert_buff_to_frame(xdp);
>>>>>>>> +        if (unlikely(!xdpf)) {
>>>>>>>> + trace_xdp_exception(queue->info->netdev, prog, act);
>>>>>>>> +            break;
>>>>>>>> +        }
>>>>>> Do you mean that it would be better to move the get_page(pdata) 
>>>>>> call lower,
>>>>>> after checking for NULL in xdpf, so that the reference count is 
>>>>>> only increased
>>>>>> after a successful conversion?
>>>>>
>>>>> I think the error handling here is generally broken (or at least very
>>>>> questionable).
>>>>>
>>>>> I suspect that in case of at least some errors the get_page() is 
>>>>> leaking
>>>>> even without this new patch.
>>>>>
>>>>> In case I'm wrong a comment reasoning why there is no leak should be
>>>>> added.
>>>>>
>>>>>
>>>>> Juergen
>>>>
>>>> I think pdata is freed in xdp_return_frame_rx_napi() -> __xdp_return()
>>>
>>> Agreed. But what if xennet_xdp_xmit() returns an error < 0?
>>>
>>> In this case xdp_return_frame_rx_napi() won't be called.
>>>
>>>
>>> Juergen
>>
>> Agreed. There is no explicit freed pdata in the calling function
>> xennet_get_responses(). Without this, the page referenced by pdata
>> could be leaked.
>>
>> I suggest:
>
> Could you please merge the two if () blocks, as they share the
> call of xdp_return_frame_rx_napi() now? Something like:
>
> if (unlikely(err <= 0)) {
>     if (err < 0)
>         trace_xdp_exception(queue->info->netdev, prog, act);
>     xdp_return_frame_rx_napi(xdpf);
> }
>
> Juergen
>
> P.S.: please don't use HTML in emails

I can't do this because xennet_xdp_xmit() can return a value > 0

Re: [PATCH] xen-netfront: handle NULL returned by xdp_convert_buff_to_frame()

[Jürgen Groß]

[-- Attachment #1.1.1: Type: text/plain, Size: 2350 bytes --]

On 17.04.25 13:19, Alexey wrote:
> 
> On 17.04.2025 13:23, Jürgen Groß wrote:
>> On 17.04.25 12:06, Alexey wrote:
>>>
>>> On 17.04.2025 11:51, Juergen Gross wrote:
>>>> On 17.04.25 10:45, Alexey wrote:
>>>>>
>>>>> On 17.04.2025 10:12, Jürgen Groß wrote:
>>>>>> On 17.04.25 09:00, Alexey wrote:
>>>>>>>
>>>>>>> On 17.04.2025 03:58, Jakub Kicinski wrote:
>>>>>>>> On Mon, 14 Apr 2025 18:34:01 +0000 Alexey Nepomnyashih wrote:
>>>>>>>>>           get_page(pdata);
>>>>>>>> Please notice this get_page() here.
>>>>>>>>
>>>>>>>>>           xdpf = xdp_convert_buff_to_frame(xdp);
>>>>>>>>> +        if (unlikely(!xdpf)) {
>>>>>>>>> + trace_xdp_exception(queue->info->netdev, prog, act);
>>>>>>>>> +            break;
>>>>>>>>> +        }
>>>>>>> Do you mean that it would be better to move the get_page(pdata) call lower,
>>>>>>> after checking for NULL in xdpf, so that the reference count is only 
>>>>>>> increased
>>>>>>> after a successful conversion?
>>>>>>
>>>>>> I think the error handling here is generally broken (or at least very
>>>>>> questionable).
>>>>>>
>>>>>> I suspect that in case of at least some errors the get_page() is leaking
>>>>>> even without this new patch.
>>>>>>
>>>>>> In case I'm wrong a comment reasoning why there is no leak should be
>>>>>> added.
>>>>>>
>>>>>>
>>>>>> Juergen
>>>>>
>>>>> I think pdata is freed in xdp_return_frame_rx_napi() -> __xdp_return()
>>>>
>>>> Agreed. But what if xennet_xdp_xmit() returns an error < 0?
>>>>
>>>> In this case xdp_return_frame_rx_napi() won't be called.
>>>>
>>>>
>>>> Juergen
>>>
>>> Agreed. There is no explicit freed pdata in the calling function
>>> xennet_get_responses(). Without this, the page referenced by pdata
>>> could be leaked.
>>>
>>> I suggest:
>>
>> Could you please merge the two if () blocks, as they share the
>> call of xdp_return_frame_rx_napi() now? Something like:
>>
>> if (unlikely(err <= 0)) {
>>     if (err < 0)
>>         trace_xdp_exception(queue->info->netdev, prog, act);
>>     xdp_return_frame_rx_napi(xdpf);
>> }
>>
>> Juergen
>>
>> P.S.: please don't use HTML in emails
> 
> I can't do this because xennet_xdp_xmit() can return a value > 0

Yes, this is what the "if (unlikely(err <= 0))" is for.


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3743 bytes --]

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