[PATCH v3] staging: ks7010: Add null pointer check for skb

[PATCH v3] staging: ks7010: Add null pointer check for skb

[Aymen Qader]

Add a null pointer check for the socket buffer in ks_hostif.c to avoid a
possible null pointer deference, and remove a later now-redundant null
pointer check.

Signed-off-by: Aymen Qader <qader.aymen@gmail.com>
---
v2: Remove redundant pointer check
v3: Style fix

 drivers/staging/ks7010/ks_hostif.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/ks7010/ks_hostif.c b/drivers/staging/ks7010/ks_hostif.c
index 0e554e3359b5..95b6c7557e84 100644
--- a/drivers/staging/ks7010/ks_hostif.c
+++ b/drivers/staging/ks7010/ks_hostif.c
@@ -1011,6 +1011,11 @@ int hostif_data_request(struct ks_wlan_private *priv, struct sk_buff *skb)
 	size_t size;
 	int ret;
 
+	if (!skb) {
+		ret = -ENOMEM;
+		goto err_kfree;
+	}
+
 	skb_len = skb->len;
 	if (skb_len > ETH_FRAME_LEN) {
 		netdev_err(priv->net_dev, "bad length skb_len=%d\n", skb_len);
@@ -1023,7 +1028,6 @@ int hostif_data_request(struct ks_wlan_private *priv, struct sk_buff *skb)
 	    priv->wpa.mic_failure.stop) {
 		if (netif_queue_stopped(priv->net_dev))
 			netif_wake_queue(priv->net_dev);
-		if (skb)
 			dev_kfree_skb(skb);
 
 		return 0;
-- 
2.17.1

Re: [PATCH v3] staging: ks7010: Add null pointer check for skb

[Aymen Qader]

Retraction: in hindsight I see that with the current usage of this
function, there is already a check for the socket buffer so this check
is unnecessary. However, I'm not sure if it's considered good practice
to keep this check anyway--in any case, ENOMEM isn't the right error
to return.

On Thu, Sep 27, 2018 at 04:16:13PM +0100, Aymen Qader wrote:
> Add a null pointer check for the socket buffer in ks_hostif.c to avoid a
> possible null pointer deference, and remove a later now-redundant null
> pointer check.
> 
> Signed-off-by: Aymen Qader <qader.aymen@gmail.com>
> ---
> v2: Remove redundant pointer check
> v3: Style fix
> 
>  drivers/staging/ks7010/ks_hostif.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/ks7010/ks_hostif.c b/drivers/staging/ks7010/ks_hostif.c
> index 0e554e3359b5..95b6c7557e84 100644
> --- a/drivers/staging/ks7010/ks_hostif.c
> +++ b/drivers/staging/ks7010/ks_hostif.c
> @@ -1011,6 +1011,11 @@ int hostif_data_request(struct ks_wlan_private *priv, struct sk_buff *skb)
>  	size_t size;
>  	int ret;
>  
> +	if (!skb) {
> +		ret = -ENOMEM;
> +		goto err_kfree;
> +	}
> +
>  	skb_len = skb->len;
>  	if (skb_len > ETH_FRAME_LEN) {
>  		netdev_err(priv->net_dev, "bad length skb_len=%d\n", skb_len);
> @@ -1023,7 +1028,6 @@ int hostif_data_request(struct ks_wlan_private *priv, struct sk_buff *skb)
>  	    priv->wpa.mic_failure.stop) {
>  		if (netif_queue_stopped(priv->net_dev))
>  			netif_wake_queue(priv->net_dev);
> -		if (skb)
>  			dev_kfree_skb(skb);
>  
>  		return 0;
> -- 
> 2.17.1
> 

Re: [PATCH v3] staging: ks7010: Add null pointer check for skb

[Dan Carpenter]

You might want to try running Smatch on your patches.  This is the
second one where maybe the results would have been interesting.

git clone http://repo.or.cz/w/smatch.git
cd smatch
make
cd ~/kernel/src/
~/smatch/smatch_scripts/kchecker drivers/staging/ks7010/ks_hostif.c

This patch introduced a problem, but the earlier one fixed a potential
problem in rtl8723bs:

drivers/staging/rtl8723bs/core/rtw_mlme_ext.c:1274 OnAssocReq() error: uninitialized symbol 'ie_len'.

Unfortunately, Smatch missed the potential NULL dereference in
OnAssocReq()...  It wouldn't be that hard to fix...

regards,
dan carpenter

Re: [PATCH v3] staging: ks7010: Add null pointer check for skb

[Dan Carpenter]

Btw, if you have the cross function DB built then Smatch says that the
NULL check can be removed here no problem.

$ smdb hostif_data_request
[ snip ]
drivers/staging/ks7010/ks_wlan_net.c |   ks_wlan_start_xmit |  hostif_data_request |  PARAM_VALUE |  1 | skb | s64min-(-1),1-s64max

regards,
dan carpenter

Re: [PATCH v3] staging: ks7010: Add null pointer check for skb

[Dan Carpenter]

On Thu, Sep 27, 2018 at 07:04:43PM +0100, Aymen Qader wrote:
> Retraction: in hindsight I see that with the current usage of this
> function, there is already a check for the socket buffer so this check
> is unnecessary. However, I'm not sure if it's considered good practice
> to keep this check anyway--in any case, ENOMEM isn't the right error
> to return.

When we find inconsistent NULL checks then we fix it to make sense.
Generally, we prefer a minimal style, with no extra code for future
proofing.  (The future seldom goes the way you expect and those extra
NULL checks would be easy to add back).

So, yes, do remove the NULL check but also fix the indenting while
you're at it.

Take your time to write patches.  I write them, then I sit on them over
night then I send them in the morning.  It means that sometimes other
people have already sent it but that's fine.  If you have to redo a
patch then don't send the v2 patch on the same day.  v2 patches are
stressful and you imagine that everyone is waiting for you to send it
or something.  We are not waiting for you.  We don't care if you wait
until next week to send these...

So when you write a v2 patch wait until the next day to send it.  Then
you will be calm when you review it.

regards,
dan carpenter

Re: [PATCH v3] staging: ks7010: Add null pointer check for skb

[Aymen Qader]

On Fri, Sep 28, 2018 at 11:13:25AM +0300, Dan Carpenter wrote:
> You might want to try running Smatch on your patches.  This is the
> second one where maybe the results would have been interesting.
> 
> git clone http://repo.or.cz/w/smatch.git
> cd smatch
> make
> cd ~/kernel/src/
> ~/smatch/smatch_scripts/kchecker drivers/staging/ks7010/ks_hostif.c
> 
> This patch introduced a problem, but the earlier one fixed a potential
> problem in rtl8723bs:
> 
> drivers/staging/rtl8723bs/core/rtw_mlme_ext.c:1274 OnAssocReq() error: uninitialized symbol 'ie_len'.
> 
> Unfortunately, Smatch missed the potential NULL dereference in
> OnAssocReq()...  It wouldn't be that hard to fix...
> 
> regards,
> dan carpenter

Oh thanks, I've not used Smatch before--that's really helpful, I'm going
to use that a lot in the future.

On Fri, Sep 28, 2018 at 11:16:01AM +0300, Dan Carpenter wrote:
> Btw, if you have the cross function DB built then Smatch says that the
> NULL check can be removed here no problem.
> 
> $ smdb hostif_data_request
> [ snip ]
> drivers/staging/ks7010/ks_wlan_net.c |   ks_wlan_start_xmit |  hostif_data_request |  PARAM_VALUE |  1 | skb | s64min-(-1),1-s64max
> 
> regards,
> dan carpenter
> 

Again, thanks very much! That's really cool, Smatch looks like a great
tool, I wish I found it before!

On Fri, Sep 28, 2018 at 11:22:39AM +0300, Dan Carpenter wrote:
> On Thu, Sep 27, 2018 at 07:04:43PM +0100, Aymen Qader wrote:
> > Retraction: in hindsight I see that with the current usage of this
> > function, there is already a check for the socket buffer so this check
> > is unnecessary. However, I'm not sure if it's considered good practice
> > to keep this check anyway--in any case, ENOMEM isn't the right error
> > to return.
> 
> When we find inconsistent NULL checks then we fix it to make sense.
> Generally, we prefer a minimal style, with no extra code for future
> proofing.  (The future seldom goes the way you expect and those extra
> NULL checks would be easy to add back).

Hm I understand, so if a parameter can currently never be null there's
no need to add unnecessary checks for in case that changes in the
future. I'll be sure to keep that in mind from now on.

> 
> So, yes, do remove the NULL check but also fix the indenting while
> you're at it.
> 
> Take your time to write patches.  I write them, then I sit on them over
> night then I send them in the morning.  It means that sometimes other
> people have already sent it but that's fine.  If you have to redo a
> patch then don't send the v2 patch on the same day.  v2 patches are
> stressful and you imagine that everyone is waiting for you to send it
> or something.  We are not waiting for you.  We don't care if you wait
> until next week to send these...
> 
> So when you write a v2 patch wait until the next day to send it.  Then
> you will be calm when you review it.
> 
> regards,
> dan carpenter
>  

Thank you very much for all of these tips, I really appreciate you
taking the time. I'm definitely going to try and apply this philosophy
in the future, I think I've been too impulsive with my patches so far.
I've been thinking "oh no, I made a mistake--I need to quickly send a
v2!" and then end up making even more mistakes, so I completely
understand what you mean.

Thanks again.

Kind regards,
Aymen Qader