netdev - Re: [PATCH net] qed: fix LL2 RX buffer allocation

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date: Mon, 16 Oct 2023 09:17:13 -0700
From: Chris Leech <cleech@...hat.com>
To: Manish Chopra <manishc@...vell.com>
Cc: kuba@...nel.org, netdev@...r.kernel.org, aelior@...vell.com,
	palok@...vell.com, njavali@...vell.com, skashyap@...vell.com,
	jmeneghi@...hat.com, pabeni@...hat.com, edumazet@...gle.com,
	horms@...nel.org, Yuval.Mintz@...iumnetworks.com,
	Ram.Amrani@...iumnetworks.com,
	"David S . Miller" <davem@...emloft.net>
Subject: Re: [PATCH net] qed: fix LL2 RX buffer allocation

On Fri, Oct 13, 2023 at 06:48:12PM +0530, Manish Chopra wrote:
> Driver allocates the LL2 rx buffers from kmalloc()
> area to construct the skb using slab_build_skb()
> 
> The required size allocation seems to have overlooked
> for accounting both skb_shared_info size and device
> placement padding bytes which results into the below
> panic when doing skb_put() for a standard MTU sized frame.
> 
> skbuff: skb_over_panic: text:ffffffffc0b0225f len:1514 put:1514
> head:ff3dabceaf39c000 data:ff3dabceaf39c042 tail:0x62c end:0x566
> dev:<NULL>
> …
> skb_panic+0x48/0x4a
> skb_put.cold+0x10/0x10
> qed_ll2b_complete_rx_packet+0x14f/0x260 [qed]
> qed_ll2_rxq_handle_completion.constprop.0+0x169/0x200 [qed]
> qed_ll2_rxq_completion+0xba/0x320 [qed]
> qed_int_sp_dpc+0x1a7/0x1e0 [qed]
> 
> This patch fixes this by accouting skb_shared_info and device
> placement padding size bytes when allocating the buffers.
> 
> Cc: David S. Miller <davem@...emloft.net>
> Fixes: 0a7fb11c23c0 ("qed: Add Light L2 support")
> Signed-off-by: Manish Chopra <manishc@...vell.com>
> ---

Ack.  While normally build_skb will use the full slab object size
anyway, this skb_over_panic is seen when the buffer is allocated from a
kfence pool which changes the return value of ksize.

Reviewed-by: Chris Leech <cleech@...hat.com>

>  drivers/net/ethernet/qlogic/qed/qed_ll2.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/qlogic/qed/qed_ll2.c b/drivers/net/ethernet/qlogic/qed/qed_ll2.c
> index 717a0b3f89bd..ab5ef254a748 100644
> --- a/drivers/net/ethernet/qlogic/qed/qed_ll2.c
> +++ b/drivers/net/ethernet/qlogic/qed/qed_ll2.c
> @@ -113,7 +113,10 @@ static void qed_ll2b_complete_tx_packet(void *cxt,
>  static int qed_ll2_alloc_buffer(struct qed_dev *cdev,
>  				u8 **data, dma_addr_t *phys_addr)
>  {
> -	*data = kmalloc(cdev->ll2->rx_size, GFP_ATOMIC);
> +	size_t size = cdev->ll2->rx_size + NET_SKB_PAD +
> +		      SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
> +
> +	*data = kmalloc(size, GFP_ATOMIC);
>  	if (!(*data)) {
>  		DP_INFO(cdev, "Failed to allocate LL2 buffer data\n");
>  		return -ENOMEM;
> @@ -2589,7 +2592,7 @@ static int qed_ll2_start(struct qed_dev *cdev, struct qed_ll2_params *params)
>  	INIT_LIST_HEAD(&cdev->ll2->list);
>  	spin_lock_init(&cdev->ll2->lock);
>  
> -	cdev->ll2->rx_size = NET_SKB_PAD + ETH_HLEN +
> +	cdev->ll2->rx_size = PRM_DMA_PAD_BYTES_NUM + ETH_HLEN +
>  			     L1_CACHE_BYTES + params->mtu;
>  
>  	/* Allocate memory for LL2.
> -- 
> 2.27.0
>