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

[Xiaomeng Tong <xiam0nd.tong@gmail.com>]

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