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: [email protected]
Signed-off-by: Du Cheng <[email protected]>
---
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
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: [email protected]
> Signed-off-by: Du Cheng <[email protected]>
> ---
> 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
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: [email protected]
> > Signed-off-by: Du Cheng <[email protected]>
> > ---
> > 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
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
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