[v1] wifi: mac80211_hwsim: Prevent tsf from setting if beacon is disabled

[PATCH] wifi: mac80211_hwsim: Prevent tsf from setting if beacon is disabled

Posted by Edward Adam Davis 8 months, 1 week ago

Setting tsf is meaningless if beacon is disabled, so check that beacon
is enabled before setting tsf.

Reported-by: syzbot+064815c6cd721082a52a@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=064815c6cd721082a52a
Tested-by: syzbot+064815c6cd721082a52a@syzkaller.appspotmail.com
Signed-off-by: Edward Adam Davis <eadavis@qq.com>
---
 drivers/net/wireless/virtual/mac80211_hwsim.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/net/wireless/virtual/mac80211_hwsim.c b/drivers/net/wireless/virtual/mac80211_hwsim.c
index cf3e976471c6..cd9e89aebb83 100644
--- a/drivers/net/wireless/virtual/mac80211_hwsim.c
+++ b/drivers/net/wireless/virtual/mac80211_hwsim.c
@@ -1226,6 +1226,11 @@ static void mac80211_hwsim_set_tsf(struct ieee80211_hw *hw,
 {
 	struct mac80211_hwsim_data *data = hw->priv;
 	u64 now = mac80211_hwsim_get_tsf(hw, vif);
+	struct ieee80211_bss_conf *conf = link_conf_dereference_protected(vif,
+			data->link_data[0].link_id);
+
+	if (conf && !conf->enable_beacon)
+		return;
 	/* MLD not supported here */
 	u32 bcn_int = data->link_data[0].beacon_int;
 	u64 delta = abs(tsf - now);
-- 
2.43.0

Re: [PATCH] wifi: mac80211_hwsim: Prevent tsf from setting if beacon is disabled

Posted by Johannes Berg 7 months, 4 weeks ago

On Sun, 2025-04-13 at 14:11 +0800, Edward Adam Davis wrote:
> 
> --- a/drivers/net/wireless/virtual/mac80211_hwsim.c
> +++ b/drivers/net/wireless/virtual/mac80211_hwsim.c
> @@ -1226,6 +1226,11 @@ static void mac80211_hwsim_set_tsf(struct ieee80211_hw *hw,
>  {
>  	struct mac80211_hwsim_data *data = hw->priv;
>  	u64 now = mac80211_hwsim_get_tsf(hw, vif);
> +	struct ieee80211_bss_conf *conf = link_conf_dereference_protected(vif,
> +			data->link_data[0].link_id);
> +
> +	if (conf && !conf->enable_beacon)
> +		return;
>  	/* MLD not supported here */
>  	u32 bcn_int = data->link_data[0].beacon_int;
>  	u64 delta = abs(tsf - now);

Please keep kernel coding style - the line break there is awful (but
with "conf = ..." on a line by itself it can be just one line), and you
shouldn't have code before variable declarations.

The comment should probably also move because it's relevant for your new
[0] as well.

johannes

Re: [PATCH] wifi: mac80211_hwsim: Prevent tsf from setting if beacon is disabled

Posted by Edward Adam Davis 7 months, 4 weeks ago

On Wed, 23 Apr 2025 14:53:53 +0200, Johannes Berg wrote:
> > --- a/drivers/net/wireless/virtual/mac80211_hwsim.c
> > +++ b/drivers/net/wireless/virtual/mac80211_hwsim.c
> > @@ -1226,6 +1226,11 @@ static void mac80211_hwsim_set_tsf(struct ieee80211_hw *hw,
> >  {
> >  	struct mac80211_hwsim_data *data = hw->priv;
> >  	u64 now = mac80211_hwsim_get_tsf(hw, vif);
> > +	struct ieee80211_bss_conf *conf = link_conf_dereference_protected(vif,
> > +			data->link_data[0].link_id);
> > +
> > +	if (conf && !conf->enable_beacon)
> > +		return;
> >  	/* MLD not supported here */
> >  	u32 bcn_int = data->link_data[0].beacon_int;
> >  	u64 delta = abs(tsf - now);
> 
> Please keep kernel coding style - the line break there is awful (but
> with "conf = ..." on a line by itself it can be just one line), and you
> shouldn't have code before variable declarations.
like this?
diff --git a/drivers/net/wireless/virtual/mac80211_hwsim.c b/drivers/net/wireless/virtual/mac80211_hwsim.c
index cf3e976471c6..6ca5d9d0fe53 100644
--- a/drivers/net/wireless/virtual/mac80211_hwsim.c
+++ b/drivers/net/wireless/virtual/mac80211_hwsim.c
@@ -1229,6 +1229,11 @@ static void mac80211_hwsim_set_tsf(struct ieee80211_hw *hw,
        /* MLD not supported here */
        u32 bcn_int = data->link_data[0].beacon_int;
        u64 delta = abs(tsf - now);
+       struct ieee80211_bss_conf *conf;
+
+       conf = link_conf_dereference_protected(vif, data->link_data[0].link_id);
+       if (conf && !conf->enable_beacon)
+               return;

        /* adjust after beaconing with new timestamp at old TBTT */
        if (tsf > now) {

> 
> The comment should probably also move because it's relevant for your new
> [0] as well.
I don't understand what you mean.

Edward

Re: [PATCH] wifi: mac80211_hwsim: Prevent tsf from setting if beacon is disabled

Posted by Johannes Berg 7 months, 4 weeks ago

On Wed, 2025-04-23 at 21:56 +0800, Edward Adam Davis wrote:
> On Wed, 23 Apr 2025 14:53:53 +0200, Johannes Berg wrote:
> > > --- a/drivers/net/wireless/virtual/mac80211_hwsim.c
> > > +++ b/drivers/net/wireless/virtual/mac80211_hwsim.c
> > > @@ -1226,6 +1226,11 @@ static void mac80211_hwsim_set_tsf(struct ieee80211_hw *hw,
> > >  {
> > >  	struct mac80211_hwsim_data *data = hw->priv;
> > >  	u64 now = mac80211_hwsim_get_tsf(hw, vif);
> > > +	struct ieee80211_bss_conf *conf = link_conf_dereference_protected(vif,
> > > +			data->link_data[0].link_id);
> > > +
> > > +	if (conf && !conf->enable_beacon)
> > > +		return;
> > >  	/* MLD not supported here */
> > >  	u32 bcn_int = data->link_data[0].beacon_int;
> > >  	u64 delta = abs(tsf - now);
> > 
> > Please keep kernel coding style - the line break there is awful (but
> > with "conf = ..." on a line by itself it can be just one line), and you
> > shouldn't have code before variable declarations.
> like this?

Looks good I guess, not sure you wanted bcn_int/delta to be calculated
before or after.

> > The comment should probably also move because it's relevant for your new
> > [0] as well.
> I don't understand what you mean.

The "/* MLD not supported here */" comment refers to the [0] - it
explains why the [0] (rather than link id) is OK. So it also applies to
your [0], if you're going to put it before the comment then IMHO it
makes sense to move the comment. With what you did now the comment is
still earlier though, of course.

johannes

Re: [PATCH] wifi: mac80211_hwsim: Prevent tsf from setting if beacon is disabled

Posted by Edward Adam Davis 7 months, 4 weeks ago

On Wed, 23 Apr 2025 16:04:04 +0200, Johannes Berg wrote:
> On Wed, 2025-04-23 at 21:56 +0800, Edward Adam Davis wrote:
> > On Wed, 23 Apr 2025 14:53:53 +0200, Johannes Berg wrote:
> > > > --- a/drivers/net/wireless/virtual/mac80211_hwsim.c
> > > > +++ b/drivers/net/wireless/virtual/mac80211_hwsim.c
> > > > @@ -1226,6 +1226,11 @@ static void mac80211_hwsim_set_tsf(struct ieee80211_hw *hw,
> > > >  {
> > > >  	struct mac80211_hwsim_data *data = hw->priv;
> > > >  	u64 now = mac80211_hwsim_get_tsf(hw, vif);
> > > > +	struct ieee80211_bss_conf *conf = link_conf_dereference_protected(vif,
> > > > +			data->link_data[0].link_id);
> > > > +
> > > > +	if (conf && !conf->enable_beacon)
> > > > +		return;
> > > >  	/* MLD not supported here */
> > > >  	u32 bcn_int = data->link_data[0].beacon_int;
> > > >  	u64 delta = abs(tsf - now);
> > >
> > > Please keep kernel coding style - the line break there is awful (but
> > > with "conf = ..." on a line by itself it can be just one line), and you
> > > shouldn't have code before variable declarations.
> > like this?
> 
> Looks good I guess, not sure you wanted bcn_int/delta to be calculated
> before or after.
It will be all right.
I will send V2 patch.
> 
> > > The comment should probably also move because it's relevant for your new
> > > [0] as well.
> > I don't understand what you mean.
> 
> The "/* MLD not supported here */" comment refers to the [0] - it
> explains why the [0] (rather than link id) is OK. So it also applies to
> your [0], if you're going to put it before the comment then IMHO it
> makes sense to move the comment. With what you did now the comment is
> still earlier though, of course.
Oh, Got it.

Edward

[PATCH V2] wifi: mac80211_hwsim: Prevent tsf from setting if beacon is disabled

Posted by Edward Adam Davis 7 months, 4 weeks ago

Setting tsf is meaningless if beacon is disabled, so check that beacon
is enabled before setting tsf.

Reported-by: syzbot+064815c6cd721082a52a@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=064815c6cd721082a52a
Tested-by: syzbot+064815c6cd721082a52a@syzkaller.appspotmail.com
Signed-off-by: Edward Adam Davis <eadavis@qq.com>
---
V1 -> V2: Move initialization and judgment to after delta.

 drivers/net/wireless/virtual/mac80211_hwsim.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/net/wireless/virtual/mac80211_hwsim.c b/drivers/net/wireless/virtual/mac80211_hwsim.c
index cf3e976471c6..6ca5d9d0fe53 100644
--- a/drivers/net/wireless/virtual/mac80211_hwsim.c
+++ b/drivers/net/wireless/virtual/mac80211_hwsim.c
@@ -1229,6 +1229,11 @@ static void mac80211_hwsim_set_tsf(struct ieee80211_hw *hw,
 	/* MLD not supported here */
 	u32 bcn_int = data->link_data[0].beacon_int;
 	u64 delta = abs(tsf - now);
+	struct ieee80211_bss_conf *conf;
+
+	conf = link_conf_dereference_protected(vif, data->link_data[0].link_id);
+	if (conf && !conf->enable_beacon)
+		return;
 
 	/* adjust after beaconing with new timestamp at old TBTT */
 	if (tsf > now) {
-- 
2.43.0