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: <MW2PR07MB4139EC8AD7690DA00E5639F58A700@MW2PR07MB4139.namprd07.prod.outlook.com>
Date:   Tue, 19 Jun 2018 06:42:04 +0000
From:   "Kalluru, Sudarsana" <Sudarsana.Kalluru@...ium.com>
To:     Yunsheng Lin <linyunsheng@...wei.com>,
        "davem@...emloft.net" <davem@...emloft.net>
CC:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "Elior, Ariel" <Ariel.Elior@...ium.com>,
        "Kalderon, Michal" <Michal.Kalderon@...ium.com>
Subject: RE: [PATCH net 1/3] qed: Fix possible memory leak in Rx error path
 handling.



-----Original Message-----
From: Yunsheng Lin [mailto:linyunsheng@...wei.com] 
Sent: 19 June 2018 11:32
To: Kalluru, Sudarsana <Sudarsana.Kalluru@...ium.com>; davem@...emloft.net
Cc: netdev@...r.kernel.org; Elior, Ariel <Ariel.Elior@...ium.com>; Kalderon, Michal <Michal.Kalderon@...ium.com>
Subject: Re: [PATCH net 1/3] qed: Fix possible memory leak in Rx error path handling.

External Email

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?

[Sudarsana] Thanks for reviewing the changes. qed_ll2_alloc_buffer() allocates this memory. The allocated buffer (i.e., buffer->data) holds complete memory for 'skb + data' as required by build_skb() implementation. Hence freeing of (buffer->data) would suffice here. 

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