[PATCH] carl9170: tx: fix an incorrect use of list iterator

[PATCH] carl9170: tx: fix an incorrect use of list iterator

[Xiaomeng Tong]

If the previous list_for_each_entry_continue_rcu() don't exit early
(no goto hit inside the loop), the iterator 'cvif' after the loop
will be a bogus pointer to an invalid structure object containing
the HEAD (&ar->vif_list). As a result, the use of 'cvif' after that
will lead to a invalid memory access (i.e., 'cvif->id': the invalid
pointer dereference when return back to/after the callsite in the
carl9170_update_beacon()).

The original intention should have been to return the valid 'cvif'
when found in list, NULL otherwise. So just make 'cvif' NULL when
no entry found, to fix this bug.

Cc: stable@vger.kernel.org
Fixes: 1f1d9654e183c ("carl9170: refactor carl9170_update_beacon")
Signed-off-by: Xiaomeng Tong <xiam0nd.tong@gmail.com>
---
 drivers/net/wireless/ath/carl9170/tx.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/wireless/ath/carl9170/tx.c b/drivers/net/wireless/ath/carl9170/tx.c
index 1b76f4434c06..2b8084121001 100644
--- a/drivers/net/wireless/ath/carl9170/tx.c
+++ b/drivers/net/wireless/ath/carl9170/tx.c
@@ -1558,6 +1558,9 @@ static struct carl9170_vif_info *carl9170_pick_beaconing_vif(struct ar9170 *ar)
 					goto out;
 			}
 		} while (ar->beacon_enabled && i--);
+
+		/* no entry found in list */
+		cvif = NULL;
 	}
 
 out:
-- 
2.17.1

Re: [PATCH] carl9170: tx: fix an incorrect use of list iterator

[Christian Lamparter]

Hi,

On Sun, Mar 27, 2022 at 8:10 PM Xiaomeng Tong <xiam0nd.tong@gmail.com> wrote:
>
> If the previous list_for_each_entry_continue_rcu() don't exit early
> (no goto hit inside the loop), the iterator 'cvif' after the loop
> will be a bogus pointer to an invalid structure object containing
> the HEAD (&ar->vif_list). As a result, the use of 'cvif' after that
> will lead to a invalid memory access (i.e., 'cvif->id': the invalid
> pointer dereference when return back to/after the callsite in the
> carl9170_update_beacon()).
>
> The original intention should have been to return the valid 'cvif'
> when found in list, NULL otherwise. So just make 'cvif' NULL when
> no entry found, to fix this bug.
>
> Cc: stable@vger.kernel.org
> Fixes: 1f1d9654e183c ("carl9170: refactor carl9170_update_beacon")
> Signed-off-by: Xiaomeng Tong <xiam0nd.tong@gmail.com>
> ---
>  drivers/net/wireless/ath/carl9170/tx.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/net/wireless/ath/carl9170/tx.c b/drivers/net/wireless/ath/carl9170/tx.c
> index 1b76f4434c06..2b8084121001 100644
> --- a/drivers/net/wireless/ath/carl9170/tx.c
> +++ b/drivers/net/wireless/ath/carl9170/tx.c
> @@ -1558,6 +1558,9 @@ static struct carl9170_vif_info *carl9170_pick_beaconing_vif(struct ar9170 *ar)
>                                         goto out;
>                         }
>                 } while (ar->beacon_enabled && i--);
> +
> +               /* no entry found in list */
> +               cvif = NULL;
>         }
>
>  out:

hmm, It's really not easy test this.  There are multiple locks, device
states and flags to consider.
the state of the protecting ar->vifs > 0 (I guess this could be > 1.
There's no point in being choosy
about "picky choices", if there's only one), the main virtual
interface as well as cvif->enable_beacon
and ar->beacon_enable don't change on a whim.

But it it is true that this function gets called by the firmware as a
callback to the TBTT,
so it would warrant any protection that is possible. Whenever a bug
can happen or be
forced in this case: I don't know, I can't do experiments until much
later (easter :( ).
But it's better and easy to err on the side of caution.

Note: That "cvif = NULL;" will certainly break the beaconing for good
(for the remaining
lifetime of the main interface). The driver would need to be stopped
and restarted before
beaconing would work again. A safer choice would be to return NULL;
That said, if the
bug can happen, the driver/firmware would be in a bad state at that
point anyway.
So a call to carl9170_restart(ar, ...there's no code for a
driver/firmware mismatch yet) will
be necessary in the hope that this was just a temporary glitch.

Cheers,
Christian

Re: [PATCH] carl9170: tx: fix an incorrect use of list iterator

[Xiaomeng Tong]

On Sun, 27 Mar 2022 23:40:46 +0200, Christian Lamparter wrote:
> On Sun, Mar 27, 2022 at 8:10 PM Xiaomeng Tong <xiam0nd.tong@gmail.com> wrote:
> >
> > If the previous list_for_each_entry_continue_rcu() don't exit early
> > (no goto hit inside the loop), the iterator 'cvif' after the loop
> > will be a bogus pointer to an invalid structure object containing
> > the HEAD (&ar->vif_list). As a result, the use of 'cvif' after that
> > will lead to a invalid memory access (i.e., 'cvif->id': the invalid
> > pointer dereference when return back to/after the callsite in the
> > carl9170_update_beacon()).
> >
> > The original intention should have been to return the valid 'cvif'
> > when found in list, NULL otherwise. So just make 'cvif' NULL when
> > no entry found, to fix this bug.
> >
> > Cc: stable@vger.kernel.org
> > Fixes: 1f1d9654e183c ("carl9170: refactor carl9170_update_beacon")
> > Signed-off-by: Xiaomeng Tong <xiam0nd.tong@gmail.com>
> > ---
> >  drivers/net/wireless/ath/carl9170/tx.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/net/wireless/ath/carl9170/tx.c b/drivers/net/wireless/ath/carl9170/tx.c
> > index 1b76f4434c06..2b8084121001 100644
> > --- a/drivers/net/wireless/ath/carl9170/tx.c
> > +++ b/drivers/net/wireless/ath/carl9170/tx.c
> > @@ -1558,6 +1558,9 @@ static struct carl9170_vif_info *carl9170_pick_beaconing_vif(struct ar9170 *ar)
> >                                         goto out;
> >                         }
> >                 } while (ar->beacon_enabled && i--);
> > +
> > +               /* no entry found in list */
> > +               cvif = NULL;
> >         }
> >
> >  out:
> 
> hmm, It's really not easy test this.  There are multiple locks, device
> states and flags to consider.
> the state of the protecting ar->vifs > 0 (I guess this could be > 1.
> There's no point in being choosy
> about "picky choices", if there's only one), the main virtual
> interface as well as cvif->enable_beacon
> and ar->beacon_enable don't change on a whim.
> 
> But it it is true that this function gets called by the firmware as a
> callback to the TBTT,
> so it would warrant any protection that is possible. Whenever a bug
> can happen or be
> forced in this case: I don't know, I can't do experiments until much
> later (easter :( ).
> But it's better and easy to err on the side of caution.
> 
> Note: That "cvif = NULL;" will certainly break the beaconing for good
> (for the remaining
> lifetime of the main interface). The driver would need to be stopped
> and restarted before
> beaconing would work again. A safer choice would be to return NULL;
> That said, if the
> bug can happen, the driver/firmware would be in a bad state at that
> point anyway.
> So a call to carl9170_restart(ar, ...there's no code for a
> driver/firmware mismatch yet) will
> be necessary in the hope that this was just a temporary glitch.

Thank you, i have resubmitted a v2 patch as you suggested: just return NULL
to fix it. Please check it.

--
Xiaomeng Tong