linux-kernel - Re: [PATCH] staging: rtl8188eu: Avoid null pointer arithmetic

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:   Thu, 27 Sep 2018 15:52:53 -0500
From:   Larry Finger <Larry.Finger@...inger.net>
To:     Aymen Qader <qader.aymen@...il.com>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        devel@...verdev.osuosl.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] staging: rtl8188eu: Avoid null pointer arithmetic

On 9/27/18 12:04 PM, Aymen Qader wrote:
> Avoid null pointer arithmetic in rtw_mlme_ext.c by skipping other field
> checks if the information element pointer is null.
> 
> Signed-off-by: Aymen Qader <qader.aymen@...il.com>
> ---
>   drivers/staging/rtl8188eu/core/rtw_mlme_ext.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/rtl8188eu/core/rtw_mlme_ext.c b/drivers/staging/rtl8188eu/core/rtw_mlme_ext.c
> index 834053a0ae9d..8a3a71456cd0 100644
> --- a/drivers/staging/rtl8188eu/core/rtw_mlme_ext.c
> +++ b/drivers/staging/rtl8188eu/core/rtw_mlme_ext.c
> @@ -2971,8 +2971,10 @@ static unsigned int OnAssocReq(struct adapter *padapter,
>   	/*  checking SSID */
>   	p = rtw_get_ie(pframe + WLAN_HDR_A3_LEN + ie_offset, _SSID_IE_, &ie_len,
>   		pkt_len - WLAN_HDR_A3_LEN - ie_offset);
> -	if (!p)
> +	if (!p) {
>   		status = _STATS_FAILURE_;
> +		goto OnAssocReqFail;
> +	}
>   
>   	if (ie_len == 0) { /*  broadcast ssid, however it is not allowed in assocreq */
>   		status = _STATS_FAILURE_;

I do not think this patch avoids any pointer arithmetic. If p is NULL, then 
ie_len will be zero and the branch with the memcmp() call, where the pointer 
arithmetic is done, will be skipped.

That said, it is better to bail out with the first failure condition. I do not 
require the following, but the code would be even simpler if you test p and 
ie_len==0 in a single if statement and eliminate some code as in

diff --git a/drivers/staging/rtl8188eu/core/rtw_mlme_ext.c 
b/drivers/staging/rtl8188eu/core/rtw_mlme_ext.c
index 1115050077e4..71722cec84a0 100644
--- a/drivers/staging/rtl8188eu/core/rtw_mlme_ext.c
+++ b/drivers/staging/rtl8188eu/core/rtw_mlme_ext.c
@@ -2982,11 +2982,10 @@ static unsigned int OnAssocReq(struct adapter *padapter,
         /*  checking SSID */
         p = rtw_get_ie(pframe + WLAN_HDR_A3_LEN + ie_offset, _SSID_IE_, &ie_len,
                 pkt_len - WLAN_HDR_A3_LEN - ie_offset);
-       if (!p)
-               status = _STATS_FAILURE_;

-       if (ie_len == 0) { /*  broadcast ssid, however it is not allowed in 
assocreq */
+       if (!p || ie_len == 0) { /*  broadcast ssid, however it is not allowed 
in assocreq */
                 status = _STATS_FAILURE_;
+               goto OnAssocReqFail;
         } else {
                 /*  check if ssid match */
                 if (memcmp((void *)(p+2), cur->Ssid.Ssid, cur->Ssid.SsidLength))


ACKed-by: Larry Finger <Larry.Finger@...inger.net>

Thanks,

Larry