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]

Date:   Tue, 19 Jun 2018 15:24:31 +0800
From:   Yunsheng Lin <linyunsheng@...wei.com>
To:     "Kalluru, Sudarsana" <Sudarsana.Kalluru@...ium.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.



On 2018/6/19 14:42, Kalluru, Sudarsana wrote:
> 
> 
> -----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. 

As I read through the code, the skb itself is allocated by kmem_cache_alloc,
see below:

build_skb -> __build_skb -> kmem_cache_alloc

Hope I am not missing something here.

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