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

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

[Xiaomeng Tong]

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(-)

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);
-- 
2.17.1

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

[Christian Lamparter]

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

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

[Xiaomeng Tong]

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