| Message ID | 20220327072947.10744-1-xiam0nd.tong@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | carl9170: tx: fix an incorrect use of list iterator | expand |
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
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
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:
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(+)