staging: r8188eu: fix a potential NULL pointer dereference

PATCH In review Vihas Makwana 3 participants 5 messages Apr 23, 2022 – Apr 25, 2022 Staging

Share & tools

Vihas Makwana

author

2022-04-23 18:48 UTC #0 lore raw

PATCH 1/1 staging: r8188eu: fix a potential NULL pointer dereference

recvframe_chk_defrag() performs a NULL check on psta, but if that check fails then it dereferences it, which it shouldn't do as psta is NULL.

Set pdefrag_q to NULL if above check fails and let the code after it handle that case.

Fixes 1cc18a22b96b ("staging: r8188eu: Add files for new driver - part 5")

Signed-off-by Vihas Makwana <[email protected]>

---
 drivers/staging/r8188eu/core/rtw_recv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/staging/r8188eu/core/rtw_recv.c +1 -1

index c1005ddaa..db54bceff 100644
@@ -1244,7 +1244,7 @@ struct recv_frame *recvframe_chk_defrag(struct adapter *padapter, struct recv_fr
 			pdefrag_q = NULL;
 		}
 	} else {
-		pdefrag_q = &psta->sta_recvpriv.defrag_q;
+		pdefrag_q = NULL;
 	}
 
 	if ((ismfrag == 0) && (fragnum == 0))

Signature · 1 line

2.30.2

Fabio M. De Francesco

2022-04-23 23:47 UTC #1 lore raw

Quoted 4 lines · On sabato 23 aprile 2022 20:47:48 CEST Vihas Makwana wrote:

recvframe_chk_defrag() performs a NULL check on psta, but if that check fails then it dereferences it, which it shouldn't do as psta is NULL.
Set pdefrag_q to NULL if above check fails and let the code after it

handle

Quoted 3 lines

that case.
Fixes 1cc18a22b96b ("staging: r8188eu: Add files for new driver - part

5")

drivers/staging/

Quoted 6 lines

Signed-off-by Vihas Makwana <[email protected]>
---
 drivers/staging/r8188eu/core/rtw_recv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/staging/ renamed from drivers/staging/r8188eu/core/rtw_recv.c +0 -0

r8188eu/core/rtw_recv.c

drivers/staging/r8188eu/core/rtw_recv.c struct recv_frame *recvframe_chk_defrag(struct

Quoted 4 lines

index c1005ddaa..db54bceff 100644
drivers/staging/r8188eu/core/rtw_recv.c +0 -0
@@ -1244,7 +1244,7 @@ struct recv_frame *recvframe_chk_defrag(struct 

adapter *padapter, struct recv_fr

drivers/staging/r8188eu/core/rtw_recv.c struct recv_frame *recvframe_chk_defrag(struct adapter *padapter, struct recv_fr · line 1247

Quoted 5 lines

drivers/staging/r8188eu/core/rtw_recv.c +1 -1

 			pdefrag_q = NULL;
 		}
 	} else {
-		pdefrag_q = &psta->sta_recvpriv.defrag_q;
+		pdefrag_q = NULL;

Hi Vihas,

To me the code looks like this...

	struct sta_info *psta;
	...
	psta = rtw_get_stainfo(pstapriv, psta_addr);
	/* The code is about to test if "psta" is a valid pointer */
	if (!psta) {
		/* "psta" is NULL */		
		...
	} else {
		/* "psta" is not NULL */
		...

drivers/staging/r8188eu/core/rtw_recv.c struct recv_frame *recvframe_chk_defrag(struct · line 1249

Quoted 2 lines

drivers/staging/r8188eu/core/rtw_recv.c +0 -0
 	}
 

Also, even if "psta" were NULL (but it isn't), your change would still be no good.

Please be very careful with these types of changes next time :)

Thanks,

Fabio M. De Francesco

Quoted 4 lines

drivers/staging/r8188eu/core/rtw_recv.c +0 -0
 	if ((ismfrag == 0) && (fragnum == 0))
Signature · 1 line
2.30.2

Vihas Makwana

author

2022-04-24 09:27 UTC #2 lore raw

Quoted 47 lines · Fabio M. De Francesco

Quoted 4 lines · On sabato 23 aprile 2022 20:47:48 CEST Vihas Makwana wrote:
recvframe_chk_defrag() performs a NULL check on psta, but if that check fails then it dereferences it, which it shouldn't do as psta is NULL.
Set pdefrag_q to NULL if above check fails and let the code after it
handle
Quoted 3 lines
that case.
Fixes 1cc18a22b96b ("staging: r8188eu: Add files for new driver - part
5")
drivers/staging/
Quoted 6 lines
Signed-off-by Vihas Makwana <[email protected]>
---
 drivers/staging/r8188eu/core/rtw_recv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/staging/ renamed from drivers/staging/r8188eu/core/rtw_recv.c +0 -0
r8188eu/core/rtw_recv.c
drivers/staging/r8188eu/core/rtw_recv.c struct recv_frame *recvframe_chk_defrag(struct
Quoted 4 lines
index c1005ddaa..db54bceff 100644
drivers/staging/r8188eu/core/rtw_recv.c +0 -0
@@ -1244,7 +1244,7 @@ struct recv_frame *recvframe_chk_defrag(struct
adapter *padapter, struct recv_fr
drivers/staging/r8188eu/core/rtw_recv.c struct recv_frame *recvframe_chk_defrag(struct adapter *padapter, struct recv_fr · line 1247
Quoted 5 lines
drivers/staging/r8188eu/core/rtw_recv.c +1 -1
                      pdefrag_q = NULL;
              }
      } else {
-             pdefrag_q = &psta->sta_recvpriv.defrag_q;
+             pdefrag_q = NULL;
Hi Vihas,
To me the code looks like this...
        struct sta_info *psta;
        ...
        psta = rtw_get_stainfo(pstapriv, psta_addr);
        /* The code is about to test if "psta" is a valid pointer */
        if (!psta) {
                /* "psta" is NULL */
                ...
        } else {
                /* "psta" is not NULL */
                ...
drivers/staging/r8188eu/core/rtw_recv.c struct recv_frame *recvframe_chk_defrag(struct · line 1249
Quoted 2 lines
drivers/staging/r8188eu/core/rtw_recv.c +0 -0
      }
 

Quoted 11 lines

Also, even if "psta" were NULL (but it isn't), your change would still be no good.
Please be very careful with these types of changes next time :)
Thanks,
Fabio M. De Francesco
Quoted 2 lines
drivers/staging/r8188eu/core/rtw_recv.c +0 -1
      if ((ismfrag == 0) && (fragnum == 0))
--

Oh yea, sorry about this. I don't know how I missed this. Thanks for clarification Fabio. Will take care next time.

Signature · 2 lines

Thanks,
Vihas

Vihas Makwana

author

2022-04-24 09:48 UTC #3 lore raw

Please ignore this patch, Greg.

Dan Carpenter

2022-04-25 08:23 UTC #4 lore raw

In reply to Vihas Makwana

Quoted 17 lines · Vihas Makwana

recvframe_chk_defrag() performs a NULL check on psta, but if that check fails then it dereferences it, which it shouldn't do as psta is NULL.
Set pdefrag_q to NULL if above check fails and let the code after it handle that case.
Fixes 1cc18a22b96b ("staging: r8188eu: Add files for new driver - part 5")
Signed-off-by Vihas Makwana <[email protected]>
---
 drivers/staging/r8188eu/core/rtw_recv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/staging/r8188eu/core/rtw_recv.c +0 -0
index c1005ddaa..db54bceff 100644

drivers/staging/r8188eu/core/rtw_recv.c struct recv_frame *recvframe_chk_defrag(struct adapter *padapter, struct recv_fr · line 1247

Quoted 5 lines

drivers/staging/r8188eu/core/rtw_recv.c +1 -1

@@ -1244,7 +1244,7 @@ struct recv_frame *recvframe_chk_defrag(struct adapter *padapter, struct recv_fr
 			pdefrag_q = NULL;
 		}
 	} else {
-		pdefrag_q = &psta->sta_recvpriv.defrag_q;
+		pdefrag_q = NULL;

What? "psta" is valid pointer on the else path.

(Also this isn't really a dereference, this is just pointer math. It's taking the address. The Oops would happen later if psta were NULL.)

regards,
dan carpenter