Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Xiaomeng Tong <xiam0nd.tong@gmail.com>
To:     chunkeey@gmail.com
Cc:     davem@davemloft.net, kuba@kernel.org, kvalo@kernel.org,
        linux-kernel@vger.kernel.org, linux-wireless@vger.kernel.org,
        linville@tuxdriver.com, netdev@vger.kernel.org, pabeni@redhat.com,
        stable@vger.kernel.org, xiam0nd.tong@gmail.com
Subject: Re: [PATCH] carl9170: tx: fix an incorrect use of list iterator
Date:   Mon, 28 Mar 2022 20:25:51 +0800
Message-Id: <20220328122551.936-1-xiam0nd.tong@gmail.com>
In-Reply-To: <CAAd0S9CR=PjEskWAi132qWB5WL1yZwzOBfV-y5m2ERzc3L_qcA@mail.gmail.com>
References: <CAAd0S9CR=PjEskWAi132qWB5WL1yZwzOBfV-y5m2ERzc3L_qcA@mail.gmail.com>
Precedence: bulk

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