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);
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
linux-stable-mirror@lists.linaro.org
-
Alexey -
Alexey Nepomnyashih
-
Jakub Kicinski -
Juergen Gross -
Jürgen Groß