| Message ID | 20240605101135.11199-1-amishin@t-argos.ru (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | c44711b78608c98a3e6b49ce91678cd0917d5349 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net,v3] liquidio: Adjust a NULL pointer handling path in lio_vf_rep_copy_packet | expand |
Hello: This patch was applied to netdev/net.git (main) by David S. Miller <davem@davemloft.net>: On Wed, 5 Jun 2024 13:11:35 +0300 you wrote: > In lio_vf_rep_copy_packet() pg_info->page is compared to a NULL value, > but then it is unconditionally passed to skb_add_rx_frag() which looks > strange and could lead to null pointer dereference. > > lio_vf_rep_copy_packet() call trace looks like: > octeon_droq_process_packets > octeon_droq_fast_process_packets > octeon_droq_dispatch_pkt > octeon_create_recv_info > ...search in the dispatch_list... > ->disp_fn(rdisp->rinfo, ...) > lio_vf_rep_pkt_recv(struct octeon_recv_info *recv_info, ...) > In this path there is no code which sets pg_info->page to NULL. > So this check looks unneeded and doesn't solve potential problem. > But I guess the author had reason to add a check and I have no such card > and can't do real test. > In addition, the code in the function liquidio_push_packet() in > liquidio/lio_core.c does exactly the same. > > [...] Here is the summary with links: - [net,v3] liquidio: Adjust a NULL pointer handling path in lio_vf_rep_copy_packet https://git.kernel.org/netdev/net/c/c44711b78608 You are awesome, thank you!
diff --git a/drivers/net/ethernet/cavium/liquidio/lio_vf_rep.c b/drivers/net/ethernet/cavium/liquidio/lio_vf_rep.c index aa6c0dfb6f1c..e26b4ed33dc8 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));
In lio_vf_rep_copy_packet() pg_info->page is compared to a NULL value, but then it is unconditionally passed to skb_add_rx_frag() which looks strange and could lead to null pointer dereference. lio_vf_rep_copy_packet() call trace looks like: octeon_droq_process_packets octeon_droq_fast_process_packets octeon_droq_dispatch_pkt octeon_create_recv_info ...search in the dispatch_list... ->disp_fn(rdisp->rinfo, ...) lio_vf_rep_pkt_recv(struct octeon_recv_info *recv_info, ...) In this path there is no code which sets pg_info->page to NULL. So this check looks unneeded and doesn't solve potential problem. But I guess the author had reason to add a check and I have no such card and can't do real test. In addition, the code in the function liquidio_push_packet() in liquidio/lio_core.c does exactly the same. Based on this, I consider the most acceptable compromise solution to adjust this issue by moving skb_add_rx_frag() into conditional scope. Found by Linux Verification Center (linuxtesting.org) with SVACE. Fixes: 1f233f327913 ("liquidio: switchdev support for LiquidIO NIC") Signed-off-by: Aleksandr Mishin <amishin@t-argos.ru> Reviewed-by: Simon Horman <horms@kernel.org> --- v1->v2: Fix incorrect 'Fixes' tag format v2->v3: Add explanation why this should be fixed, add Reviewed-by: Simon Horman <horms@kernel.org> (https://lore.kernel.org/all/20240308201911.GB603911@kernel.org/) drivers/net/ethernet/cavium/liquidio/lio_vf_rep.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-)