| Message ID | 20220327072702.10572-1-xiam0nd.tong@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | carl9170: main: fix an incorrect use of list iterator | expand |
Hi, On 27/03/2022 09:27, Xiaomeng Tong wrote: > The bug is here: > rcu_assign_pointer(ar->tx_ampdu_iter, > (struct carl9170_sta_tid *) &ar->tx_ampdu_list); yeah, so... I know there's currently a big discussion revolving around LISTs due to incoming the GNU89 to GNU11 switch. I'm not currently aware that something related to this had updated INIT_LIST_HEAD + friends. So, please tell me if there is extra information that has to be considered. > The 'ar->tx_ampdu_iter' is used as a list iterator variable > which point to a structure object containing the list HEAD > (&ar->tx_ampdu_list), not as the HEAD itself. > > The only use case of 'ar->tx_ampdu_iter' is as a base pos > for list_for_each_entry_continue_rcu in carl9170_tx_ampdu(). > If the iterator variable holds the *wrong* HEAD value here > (has not been modified elsewhere before), this will lead to > an invalid memory access. > > Using list_entry_rcu to get the right list iterator variable > and reassign it, to fix this bug. > Note: use 'ar->tx_ampdu_list.next' instead of '&ar->tx_ampdu_list' > to avoid compiler error. > > Cc: stable@vger.kernel.org > Fixes: fe8ee9ad80b28 ("carl9170: mac80211 glue and command interface") > Signed-off-by: Xiaomeng Tong <xiam0nd.tong@gmail.com> > --- > drivers/net/wireless/ath/carl9170/main.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/wireless/ath/carl9170/main.c b/drivers/net/wireless/ath/carl9170/main.c > index 49f7ee1c912b..a287937bf666 100644 > --- a/drivers/net/wireless/ath/carl9170/main.c > +++ b/drivers/net/wireless/ath/carl9170/main.c > @@ -1756,6 +1756,7 @@ static const struct ieee80211_ops carl9170_ops = { > > void *carl9170_alloc(size_t priv_size) > { > + struct carl9170_sta_tid *tid_info; > struct ieee80211_hw *hw; > struct ar9170 *ar; > struct sk_buff *skb; > @@ -1815,8 +1816,9 @@ void *carl9170_alloc(size_t priv_size) > INIT_DELAYED_WORK(&ar->stat_work, carl9170_stat_work); > INIT_DELAYED_WORK(&ar->tx_janitor, carl9170_tx_janitor); > INIT_LIST_HEAD(&ar->tx_ampdu_list); > - rcu_assign_pointer(ar->tx_ampdu_iter, > - (struct carl9170_sta_tid *) &ar->tx_ampdu_list); > + tid_info = list_entry_rcu(ar->tx_ampdu_list.next, > + struct carl9170_sta_tid, list); > + rcu_assign_pointer(ar->tx_ampdu_iter, tid_info); I've tested this. I've added the following pr_info that would print the (raw) pointer of both your new method (your patch) and the old (current code) one: pr_info("new:%px\n", list_entry_rcu(ar->tx_ampdu_list.next,struct carl9170_sta_tid, list)); // tid_info pr_info("old:%px\n", (struct carl9170_sta_tid *) &ar->tx_ampdu_list); and run it on AR9170 USB Stick [ 216.547932] usb 2-10: SerialNumber: 12345 [ 216.673629] usb 2-10: reset high-speed USB device number 10 using xhci_hcd [ 216.853488] new:ffff9394268a38e0 [ 216.853496] old:ffff9394268a38e0 [ 216.858174] usb 2-10: driver API: 1.9.9 2016-02-15 [1-1] [ 216.858186] usb 2-10: firmware API: 1.9.9 2021-02-05 phew, what a relieve :). Both the new and old pointers are the same. So, the tx_ampdu_list is empty, as it was just initialized to (list->next = list->prev = list). And you are right about the iter being suspeciously bogus. But I think this is true for both the new and the old way. There is no real carl9170_sta_tid* tid associated with that empty entry and if some code would expect a valid carl9170_sta_tid* there, it would certainly cause crashes&burns. The carl9170_tx_ampdu() and carl9170_ampdu_gc() code is really careful though and checks whenever the list is empty or not before doing any list traversing with the tx_ampdu_iter. Any thoughts or insights? Cheers, Christian
On Sun, 27 Mar 2022 14:05:29, Christian Lamparter <chunkeey@gmail.com> wrote: > Hi, > > On 27/03/2022 09:27, Xiaomeng Tong wrote: > > The bug is here: > > rcu_assign_pointer(ar->tx_ampdu_iter, > > (struct carl9170_sta_tid *) &ar->tx_ampdu_list); > > yeah, so... I know there's currently a big discussion revolving > around LISTs due to incoming the GNU89 to GNU11 switch. I'm not > currently aware that something related to this had updated > INIT_LIST_HEAD + friends. So, please tell me if there is extra > information that has to be considered. > > > The 'ar->tx_ampdu_iter' is used as a list iterator variable > > which point to a structure object containing the list HEAD > > (&ar->tx_ampdu_list), not as the HEAD itself. > > > > The only use case of 'ar->tx_ampdu_iter' is as a base pos > > for list_for_each_entry_continue_rcu in carl9170_tx_ampdu(). > > If the iterator variable holds the *wrong* HEAD value here > > (has not been modified elsewhere before), this will lead to > > an invalid memory access. > > > > Using list_entry_rcu to get the right list iterator variable > > and reassign it, to fix this bug. > > Note: use 'ar->tx_ampdu_list.next' instead of '&ar->tx_ampdu_list' > > to avoid compiler error. > > > > Cc: stable@vger.kernel.org > > Fixes: fe8ee9ad80b28 ("carl9170: mac80211 glue and command interface") > > Signed-off-by: Xiaomeng Tong <xiam0nd.tong@gmail.com> > > --- > > drivers/net/wireless/ath/carl9170/main.c | 6 ++++-- > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/net/wireless/ath/carl9170/main.c b/drivers/net/wireless/ath/carl9170/main.c > > index 49f7ee1c912b..a287937bf666 100644 > > --- a/drivers/net/wireless/ath/carl9170/main.c > > +++ b/drivers/net/wireless/ath/carl9170/main.c > > @@ -1756,6 +1756,7 @@ static const struct ieee80211_ops carl9170_ops = { > > > > void *carl9170_alloc(size_t priv_size) > > { > > + struct carl9170_sta_tid *tid_info; > > struct ieee80211_hw *hw; > > struct ar9170 *ar; > > struct sk_buff *skb; > > @@ -1815,8 +1816,9 @@ void *carl9170_alloc(size_t priv_size) > > INIT_DELAYED_WORK(&ar->stat_work, carl9170_stat_work); > > INIT_DELAYED_WORK(&ar->tx_janitor, carl9170_tx_janitor); > > INIT_LIST_HEAD(&ar->tx_ampdu_list); > > - rcu_assign_pointer(ar->tx_ampdu_iter, > > - (struct carl9170_sta_tid *) &ar->tx_ampdu_list); > > + tid_info = list_entry_rcu(ar->tx_ampdu_list.next, > > + struct carl9170_sta_tid, list); > > + rcu_assign_pointer(ar->tx_ampdu_iter, tid_info); > > > I've tested this. I've added the following pr_info that would > print the (raw) pointer of both your new method (your patch) > and the old (current code) one: > > pr_info("new:%px\n", list_entry_rcu(ar->tx_ampdu_list.next,struct carl9170_sta_tid, list)); // tid_info > pr_info("old:%px\n", (struct carl9170_sta_tid *) &ar->tx_ampdu_list); > > and run it on AR9170 USB Stick > > [ 216.547932] usb 2-10: SerialNumber: 12345 > [ 216.673629] usb 2-10: reset high-speed USB device number 10 using xhci_hcd > [ 216.853488] new:ffff9394268a38e0 > [ 216.853496] old:ffff9394268a38e0 > [ 216.858174] usb 2-10: driver API: 1.9.9 2016-02-15 [1-1] > [ 216.858186] usb 2-10: firmware API: 1.9.9 2021-02-05 > > phew, what a relieve :). Both the new and old pointers are the same. > I double check it, and this should not be a bug, at least for now. the 'list' happens to be the first member of struct carl9170_sta_tid, so both the new and old pointers are the same. However, we should not depend on the member order of structure or compiler to guarantee the correctness, in case some day the order is changed by accident. Instead, we should guarantee it using the correct logic explicitly. -- Xiaomeng Tong
diff --git a/drivers/net/wireless/ath/carl9170/main.c b/drivers/net/wireless/ath/carl9170/main.c index 49f7ee1c912b..a287937bf666 100644 --- a/drivers/net/wireless/ath/carl9170/main.c +++ b/drivers/net/wireless/ath/carl9170/main.c @@ -1756,6 +1756,7 @@ static const struct ieee80211_ops carl9170_ops = { void *carl9170_alloc(size_t priv_size) { + struct carl9170_sta_tid *tid_info; struct ieee80211_hw *hw; struct ar9170 *ar; struct sk_buff *skb; @@ -1815,8 +1816,9 @@ void *carl9170_alloc(size_t priv_size) INIT_DELAYED_WORK(&ar->stat_work, carl9170_stat_work); INIT_DELAYED_WORK(&ar->tx_janitor, carl9170_tx_janitor); INIT_LIST_HEAD(&ar->tx_ampdu_list); - rcu_assign_pointer(ar->tx_ampdu_iter, - (struct carl9170_sta_tid *) &ar->tx_ampdu_list); + tid_info = list_entry_rcu(ar->tx_ampdu_list.next, + struct carl9170_sta_tid, list); + rcu_assign_pointer(ar->tx_ampdu_iter, tid_info); bitmap_zero(&ar->vif_bitmap, ar->fw.vif_num); INIT_LIST_HEAD(&ar->vif_list);
The bug is here: rcu_assign_pointer(ar->tx_ampdu_iter, (struct carl9170_sta_tid *) &ar->tx_ampdu_list); The 'ar->tx_ampdu_iter' is used as a list iterator variable which point to a structure object containing the list HEAD (&ar->tx_ampdu_list), not as the HEAD itself. The only use case of 'ar->tx_ampdu_iter' is as a base pos for list_for_each_entry_continue_rcu in carl9170_tx_ampdu(). If the iterator variable holds the *wrong* HEAD value here (has not been modified elsewhere before), this will lead to an invalid memory access. Using list_entry_rcu to get the right list iterator variable and reassign it, to fix this bug. Note: use 'ar->tx_ampdu_list.next' instead of '&ar->tx_ampdu_list' to avoid compiler error. Cc: stable@vger.kernel.org Fixes: fe8ee9ad80b28 ("carl9170: mac80211 glue and command interface") Signed-off-by: Xiaomeng Tong <xiam0nd.tong@gmail.com> --- drivers/net/wireless/ath/carl9170/main.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)