netdev - Re: [PATCH net 1/3] qed: Fix possible memory leak in Rx error path handling.

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <b13ecc3c-e1aa-8b87-7203-d809e5861817@huawei.com>
Date:   Tue, 19 Jun 2018 14:01:34 +0800
From:   Yunsheng Lin <linyunsheng@...wei.com>
To:     Sudarsana Reddy Kalluru <sudarsana.kalluru@...ium.com>,
        <davem@...emloft.net>
CC:     <netdev@...r.kernel.org>, <Ariel.Elior@...ium.com>,
        <Michal.Kalderon@...ium.com>
Subject: Re: [PATCH net 1/3] qed: Fix possible memory leak in Rx error path
 handling.



On 2018/6/19 12:58, Sudarsana Reddy Kalluru wrote:
> Memory for packet buffers need to be freed in the error paths as there is
> no consumer (e.g., upper layer) for such packets and that memory will never
> get freed.
> The issue was uncovered when port was attacked with flood of isatap
> packets, these are multicast packets hence were directed at all the PFs.
> For foce PF, this meant they were routed to the ll2 module which in turn
> drops such packets.
> 
> Fixes: 0a7fb11c ("qed: Add Light L2 support")
> Signed-off-by: Sudarsana Reddy Kalluru <Sudarsana.Kalluru@...ium.com>
> Signed-off-by: Ariel Elior <ariel.elior@...ium.com>
> Signed-off-by: Michal Kalderon <Michal.Kalderon@...ium.com>
> ---
>  drivers/net/ethernet/qlogic/qed/qed_ll2.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/qlogic/qed/qed_ll2.c b/drivers/net/ethernet/qlogic/qed/qed_ll2.c
> index c97ebd6..012973d 100644
> --- a/drivers/net/ethernet/qlogic/qed/qed_ll2.c
> +++ b/drivers/net/ethernet/qlogic/qed/qed_ll2.c
> @@ -201,8 +201,9 @@ void qed_ll2b_complete_rx_packet(void *cxt, struct qed_ll2_comp_rx_data *data)
>  
>  	skb = build_skb(buffer->data, 0);
>  	if (!skb) {
> -		rc = -ENOMEM;
> -		goto out_post;
> +		DP_INFO(cdev, "Failed to build SKB\n");
> +		kfree(buffer->data);
> +		goto out_post1;
>  	}
>  
>  	data->u.placement_offset += NET_SKB_PAD;
> @@ -224,8 +225,14 @@ void qed_ll2b_complete_rx_packet(void *cxt, struct qed_ll2_comp_rx_data *data)
>  		cdev->ll2->cbs->rx_cb(cdev->ll2->cb_cookie, skb,
>  				      data->opaque_data_0,
>  				      data->opaque_data_1);
> +	} else {
> +		DP_VERBOSE(p_hwfn, (NETIF_MSG_RX_STATUS | NETIF_MSG_PKTDATA |
> +				    QED_MSG_LL2 | QED_MSG_STORAGE),
> +			   "Dropping the packet\n");
> +		kfree(buffer->data);

What about the memory used by skb itself?
Does skb need to be freed by kfree_skb or something like that?

>  	}
>  
> +out_post1:
>  	/* Update Buffer information and update FW producer */
>  	buffer->data = new_data;
>  	buffer->phys_addr = new_phys_addr;
>