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

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;
> +		}

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?

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

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()

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

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); +} 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;

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

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

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