2022-11-28 10:58:18

by Aleksandr Burakov

Subject: [PATCH] liquidio: avoid NULL pointer dereference in lio_vf_rep_copy_packet()

lio_vf_rep_copy_packet() passes pg_info->page to skb_add_rx_frag()
that dereferences it without any check. So, it does not make sense
to call skb_add_rx_frag() when pg_info->page is NULL to avoid an segfault.

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

Signed-off-by: Aleksandr Burakov <[email protected]>
Fixes: 1f233f327913 ("liquidio: switchdev support for LiquidIO NIC")
---
drivers/net/ethernet/cavium/liquidio/lio_vf_rep.c | 11 +++++------
1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/cavium/liquidio/lio_vf_rep.c b/drivers/net/ethernet/cavium/liquidio/lio_vf_rep.c
index 600de587d7a9..e70b9ccca380 100644
--- a/drivers/net/ethernet/cavium/liquidio/lio_vf_rep.c
+++ b/drivers/net/ethernet/cavium/liquidio/lio_vf_rep.c
@@ -272,13 +272,12 @@ lio_vf_rep_copy_packet(struct octeon_device *oct,
pg_info->page_offset;
memcpy(skb->data, va, MIN_SKB_SIZE);
skb_put(skb, MIN_SKB_SIZE);
+ skb_add_rx_frag(skb, skb_shinfo(skb)->nr_frags,
+ pg_info->page,
+ pg_info->page_offset + MIN_SKB_SIZE,
+ len - MIN_SKB_SIZE,
+ LIO_RXBUFFER_SZ);
}
-
- skb_add_rx_frag(skb, skb_shinfo(skb)->nr_frags,
- pg_info->page,
- pg_info->page_offset + MIN_SKB_SIZE,
- len - MIN_SKB_SIZE,
- LIO_RXBUFFER_SZ);
} else {
struct octeon_skb_page_info *pg_info =
((struct octeon_skb_page_info *)(skb->cb));
--
2.25.1

2022-11-30 06:30:31

by Jakub Kicinski

Subject: Re: [PATCH] liquidio: avoid NULL pointer dereference in lio_vf_rep_copy_packet()

On Mon, 28 Nov 2022 13:26:59 +0300 Aleksandr Burakov wrote:
> --- a/drivers/net/ethernet/cavium/liquidio/lio_vf_rep.c
> +++ b/drivers/net/ethernet/cavium/liquidio/lio_vf_rep.c
> @@ -272,13 +272,12 @@ lio_vf_rep_copy_packet(struct octeon_device *oct,
> pg_info->page_offset;
> memcpy(skb->data, va, MIN_SKB_SIZE);
> skb_put(skb, MIN_SKB_SIZE);
> + skb_add_rx_frag(skb, skb_shinfo(skb)->nr_frags,
> + pg_info->page,
> + pg_info->page_offset + MIN_SKB_SIZE,
> + len - MIN_SKB_SIZE,
> + LIO_RXBUFFER_SZ);
> }
> -
> - skb_add_rx_frag(skb, skb_shinfo(skb)->nr_frags,
> - pg_info->page,
> - pg_info->page_offset + MIN_SKB_SIZE,
> - len - MIN_SKB_SIZE,
> - LIO_RXBUFFER_SZ);
> } else {
> struct octeon_skb_page_info *pg_info =
> ((struct octeon_skb_page_info *)(skb->cb));

The else branch also looks at pg_info and derefs page like there's
no tomorrow. You need to put a bit more effort into the analysis.

Marvell people please chime in and tell us what the intention is here.
Whether page can be NULL here or this is defensive programming and can
be dropped.