Subject: Re: [PATCH] net: brcm80211: fix potential NULL pointer dereferences
To:     Kangjie Lu <kjlu@umn.edu>
Cc:     pakki001@umn.edu, Franky Lin <franky.lin@broadcom.com>,
        Hante Meuleman <hante.meuleman@broadcom.com>,
        Chi-Hsien Lin <chi-hsien.lin@cypress.com>,
        Wright Feng <wright.feng@cypress.com>,
        Kalle Valo <kvalo@codeaurora.org>,
        "David S. Miller" <davem@davemloft.net>,
        =?UTF-8?B?UmFmYcWCIE1pxYJlY2tp?= <rafal@milecki.pl>,
        Stefan Wahren <stefan.wahren@i2se.com>,
        Chung-Hsien Hsu <stanley.hsu@cypress.com>,
        linux-wireless@vger.kernel.org,
        brcm80211-dev-list.pdl@broadcom.com,
        brcm80211-dev-list@cypress.com, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org
References: <20190311073232.30807-1-kjlu@umn.edu>
From:   Arend Van Spriel <arend.vanspriel@broadcom.com>
Message-ID: <9f8b2ff5-9c24-6f7a-ea7a-5b79a24fd280@broadcom.com>
Date:   Mon, 11 Mar 2019 10:11:18 +0100
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:60.0) Gecko/20100101
 Thunderbird/60.5.3
MIME-Version: 1.0
In-Reply-To: <20190311073232.30807-1-kjlu@umn.edu>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-wireless-owner@vger.kernel.org
Precedence: bulk

On 3/11/2019 8:32 AM, Kangjie Lu wrote:
> In case kmemdup fails, the fix returns -ENOMEM to avoid NULL
> pointer dereferences.

Hi Kangjie Lu,

Are you fixing any reported issue with this? If you looked further you 
would see that this function is called in two places and the return 
value is not checked there. So your patch is not changing anything.

Please sent a V2 addressing my comments below.

Thanks,
Arend

> Signed-off-by: Kangjie Lu <kjlu@umn.edu>
> ---
>   drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c | 5 +++++
>   1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
> index e92f6351bd22..d903a45e7b68 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
> @@ -5464,6 +5464,9 @@ static s32 brcmf_get_assoc_ies(struct brcmf_cfg80211_info *cfg,
>   		conn_info->req_ie =
>   		    kmemdup(cfg->extra_buf, conn_info->req_ie_len,
>   			    GFP_KERNEL);
> +		if (!conn_info->req_ie)
> +			return -ENOMEM;

No need to return an error here. Instead set conn_info->req_ie_len to 
zero here.

> +
>   	} else {
>   		conn_info->req_ie_len = 0;
>   		conn_info->req_ie = NULL;
> @@ -5480,6 +5483,8 @@ static s32 brcmf_get_assoc_ies(struct brcmf_cfg80211_info *cfg,
>   		conn_info->resp_ie =
>   		    kmemdup(cfg->extra_buf, conn_info->resp_ie_len,
>   			    GFP_KERNEL);
> +		if (!conn_info->resp_ie)
> +			return -ENOMEM;

Same here for conn_info->resp_ie_len.

>   	} else {
>   		conn_info->resp_ie_len = 0;
>   		conn_info->resp_ie = NULL;
>