lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Sun, 27 Mar 2022 23:40:46 +0200 From: Christian Lamparter <chunkeey@...il.com> To: Xiaomeng Tong <xiam0nd.tong@...il.com> Cc: Kalle Valo <kvalo@...nel.org>, David Miller <davem@...emloft.net>, Jakub Kicinski <kuba@...nel.org>, pabeni@...hat.com, "John W. Linville" <linville@...driver.com>, linux-wireless <linux-wireless@...r.kernel.org>, Netdev <netdev@...r.kernel.org>, linux-kernel <linux-kernel@...r.kernel.org>, Stable <stable@...r.kernel.org> Subject: Re: [PATCH] carl9170: tx: fix an incorrect use of list iterator Hi, On Sun, Mar 27, 2022 at 8:10 PM Xiaomeng Tong <xiam0nd.tong@...il.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@...r.kernel.org > Fixes: 1f1d9654e183c ("carl9170: refactor carl9170_update_beacon") > Signed-off-by: Xiaomeng Tong <xiam0nd.tong@...il.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
Powered by blists - more mailing lists