[PATCH] net: wireless: convert WARN_ON() to pr_warn() in cfg80211_sme_connect

[PATCH] net: wireless: convert WARN_ON() to pr_warn() in cfg80211_sme_connect

[Du Cheng]

A WARN_ON(wdev->conn) would trigger in cfg80211_sme_connect(), if
multiple send_msg() system calls are made from the userland, which
should be anticipated and handled by the wireless driver. Convert this
WARN() to pr_warn to prevent a kernel panic if kernel is configured to
"panic on warn".

Bug reported by syzbot.

Reported-by: syzbot+5f9392825de654244975@syzkaller.appspotmail.com
Signed-off-by: Du Cheng <ducheng2@gmail.com>
---
link to syzkaller:
https://syzkaller.appspot.com/bug?extid=5f9392825de654244975
this patch has passed syzbot test.
 net/wireless/sme.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/net/wireless/sme.c b/net/wireless/sme.c
index 07756ca5e3b5..87a65a4c40ae 100644
--- a/net/wireless/sme.c
+++ b/net/wireless/sme.c
@@ -529,8 +529,10 @@ static int cfg80211_sme_connect(struct wireless_dev *wdev,
 		cfg80211_sme_free(wdev);
 	}
 
-	if (WARN_ON(wdev->conn))
+	if (wdev->conn) {
+		pr_warn("%s: wdev->conn != NULL, sme connect in progress", __func__);
 		return -EINPROGRESS;
+	}
 
 	wdev->conn = kzalloc(sizeof(*wdev->conn), GFP_KERNEL);
 	if (!wdev->conn)
-- 
2.30.2

Re: [PATCH] net: wireless: convert WARN_ON() to pr_warn() in cfg80211_sme_connect

[Greg Kroah-Hartman]

On Wed, Apr 07, 2021 at 10:19:03AM +0800, Du Cheng wrote:
> A WARN_ON(wdev->conn) would trigger in cfg80211_sme_connect(), if
> multiple send_msg() system calls are made from the userland, which
> should be anticipated and handled by the wireless driver. Convert this
> WARN() to pr_warn to prevent a kernel panic if kernel is configured to
> "panic on warn".
> 
> Bug reported by syzbot.
> 
> Reported-by: syzbot+5f9392825de654244975@syzkaller.appspotmail.com
> Signed-off-by: Du Cheng <ducheng2@gmail.com>
> ---
> link to syzkaller:
> https://syzkaller.appspot.com/bug?extid=5f9392825de654244975
> this patch has passed syzbot test.
>  net/wireless/sme.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/net/wireless/sme.c b/net/wireless/sme.c
> index 07756ca5e3b5..87a65a4c40ae 100644
> --- a/net/wireless/sme.c
> +++ b/net/wireless/sme.c
> @@ -529,8 +529,10 @@ static int cfg80211_sme_connect(struct wireless_dev *wdev,
>  		cfg80211_sme_free(wdev);
>  	}
>  
> -	if (WARN_ON(wdev->conn))
> +	if (wdev->conn) {
> +		pr_warn("%s: wdev->conn != NULL, sme connect in progress", __func__);

You have a real device, please always use 'dev_warn() for stuff like
this, or the netdev equivalent.  Also no need for __func__ for stuff
like this, that's just clutter.

Also, what can a user do if they get this information?  What does it
mean to them?  Try making the text more informative.

thanks,

greg k-h

Re: [PATCH] net: wireless: convert WARN_ON() to pr_warn() in cfg80211_sme_connect

[Du Cheng]

Le Wed, Apr 07, 2021 at 07:54:22AM +0200, Greg Kroah-Hartman a écrit :
> On Wed, Apr 07, 2021 at 10:19:03AM +0800, Du Cheng wrote:
> > A WARN_ON(wdev->conn) would trigger in cfg80211_sme_connect(), if
> > multiple send_msg() system calls are made from the userland, which
> > should be anticipated and handled by the wireless driver. Convert this
> > WARN() to pr_warn to prevent a kernel panic if kernel is configured to
> > "panic on warn".
> > 
> > Bug reported by syzbot.
> > 
> > Reported-by: syzbot+5f9392825de654244975@syzkaller.appspotmail.com
> > Signed-off-by: Du Cheng <ducheng2@gmail.com>
> > ---
> > link to syzkaller:
> > https://syzkaller.appspot.com/bug?extid=5f9392825de654244975
> > this patch has passed syzbot test.
> >  net/wireless/sme.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/net/wireless/sme.c b/net/wireless/sme.c
> > index 07756ca5e3b5..87a65a4c40ae 100644
> > --- a/net/wireless/sme.c
> > +++ b/net/wireless/sme.c
> > @@ -529,8 +529,10 @@ static int cfg80211_sme_connect(struct wireless_dev *wdev,
> >  		cfg80211_sme_free(wdev);
> >  	}
> >  
> > -	if (WARN_ON(wdev->conn))
> > +	if (wdev->conn) {
> > +		pr_warn("%s: wdev->conn != NULL, sme connect in progress", __func__);
> 

Hi Greg,

Thanks for the reply.

> You have a real device, please always use 'dev_warn() for stuff like
> this, or the netdev equivalent.  Also no need for __func__ for stuff
> like this, that's just clutter.

If the warning is indeed useful here, I will change the line to dev_warn(),
however I am not sure if it is a good idea to even generate warning output as
the kernel is well able to handle this special case from the userland.
> 
> Also, what can a user do if they get this information?  What does it
> mean to them?  Try making the text more informative.
> 
> thanks,

I have spent some time to understand the netlink subsystem as a IPC mechanism.
What I have now is a reliable sequence of steps to reproduce the crash, by
condensing the syzkaller C reproducer:
[link to reproducer: https://syzkaller.appspot.com/text?tag=ReproC&x=1414c997900000]

* hwsim80211_create_device (sendmsg: HWSIM_CMD_NEW_RADIO)
* nl80211_set_interface (sendmsg: NL80211_CMD_SET_INTERFACE)
* set_interface_state (ioctl: SIOCSIFFLAGS)
* nl80211_join_ibss (sendmsg: NL80211_CMD_JOIN_IBSS)
* sendmsg: NL80211_CMD_SET_INTERFACE
* 1st sendmsg: NL80211_CMD_CONNECT
* 2nd sendmsg: NL80211_CMD_CONNECT <- this triggers the WARN_ON(wdev->conn)
* (if kernel not panic yet) more sendmsg: NL80211_CMD_CONNECT ...

If the code skips WARN_ON() and instead returns -EINPROGRESS, user application
will receive error from the following recv(sock, ...). User application can hence
choose to handle this error accordingly while kernel soldiers on without panicking.

If dev_warn() is added, for every subsequent NL80211_CMD_CONNECT, the console is
flooded with the printout.

Maybe it is ok to silently return -EINPROGRESS for the 2nd NL80211_CMD_CONNECT
and beyond.
> 
> greg k-h

Re: [PATCH] net: wireless: convert WARN_ON() to pr_warn() in cfg80211_sme_connect

[Johannes Berg]

Hi,

> I have spent some time to understand the netlink subsystem as a IPC mechanism.
> What I have now is a reliable sequence of steps to reproduce the crash, by
> condensing the syzkaller C reproducer:
> [link to reproducer: https://syzkaller.appspot.com/text?tag=ReproC&x=1414c997900000]
> 
> * hwsim80211_create_device (sendmsg: HWSIM_CMD_NEW_RADIO)
> * nl80211_set_interface (sendmsg: NL80211_CMD_SET_INTERFACE)
> * set_interface_state (ioctl: SIOCSIFFLAGS)
> * nl80211_join_ibss (sendmsg: NL80211_CMD_JOIN_IBSS)
> * sendmsg: NL80211_CMD_SET_INTERFACE
> * 1st sendmsg: NL80211_CMD_CONNECT
> * 2nd sendmsg: NL80211_CMD_CONNECT <- this triggers the WARN_ON(wdev->conn)
> * (if kernel not panic yet) more sendmsg: NL80211_CMD_CONNECT ...
> 
> If the code skips WARN_ON() and instead returns -EINPROGRESS, user application
> will receive error from the following recv(sock, ...). User application can hence
> choose to handle this error accordingly while kernel soldiers on without panicking.
> 
> If dev_warn() is added, for every subsequent NL80211_CMD_CONNECT, the console is
> flooded with the printout.
> 
> Maybe it is ok to silently return -EINPROGRESS for the 2nd NL80211_CMD_CONNECT
> and beyond.
> 

Yeah, I think the right thing to do is to just drop the WARN_ON
entirely. In fact, I can't really seem to figure out now why it was
added there (even if I probably did that myself), nothing else seems to
prevent getting to this code path multiple times directly one after
another.

johannes

Re: [PATCH] net: wireless: convert WARN_ON() to pr_warn() in cfg80211_sme_connect

[Du Cheng]

Le Wed, Apr 07, 2021 at 05:51:05PM +0200, Johannes Berg a écrit :
> Hi,
> 
> > I have spent some time to understand the netlink subsystem as a IPC mechanism.
> > What I have now is a reliable sequence of steps to reproduce the crash, by
> > condensing the syzkaller C reproducer:
> > [link to reproducer: https://syzkaller.appspot.com/text?tag=ReproC&x=1414c997900000]
> > 
> > * hwsim80211_create_device (sendmsg: HWSIM_CMD_NEW_RADIO)
> > * nl80211_set_interface (sendmsg: NL80211_CMD_SET_INTERFACE)
> > * set_interface_state (ioctl: SIOCSIFFLAGS)
> > * nl80211_join_ibss (sendmsg: NL80211_CMD_JOIN_IBSS)
> > * sendmsg: NL80211_CMD_SET_INTERFACE
> > * 1st sendmsg: NL80211_CMD_CONNECT
> > * 2nd sendmsg: NL80211_CMD_CONNECT <- this triggers the WARN_ON(wdev->conn)
> > * (if kernel not panic yet) more sendmsg: NL80211_CMD_CONNECT ...
> > 
> > If the code skips WARN_ON() and instead returns -EINPROGRESS, user application
> > will receive error from the following recv(sock, ...). User application can hence
> > choose to handle this error accordingly while kernel soldiers on without panicking.
> > 
> > If dev_warn() is added, for every subsequent NL80211_CMD_CONNECT, the console is
> > flooded with the printout.
> > 
> > Maybe it is ok to silently return -EINPROGRESS for the 2nd NL80211_CMD_CONNECT
> > and beyond.
> > 
> 
> Yeah, I think the right thing to do is to just drop the WARN_ON
> entirely. In fact, I can't really seem to figure out now why it was
> added there (even if I probably did that myself), nothing else seems to
> prevent getting to this code path multiple times directly one after
> another.
> 
> johannes
> 
Hi Johannes,

Thanks for your input! I will send a v2 that drops the WARN_ON().

Regards,
Du Cheng