From: Christian Lamparter <chunkeey@gmail.com>
To: Xiaomeng Tong <xiam0nd.tong@gmail.com>
Cc: Kalle Valo <kvalo@kernel.org>, David Miller <davem@davemloft.net>,
Jakub Kicinski <kuba@kernel.org>,
pabeni@redhat.com, "John W. Linville" <linville@tuxdriver.com>,
linux-wireless <linux-wireless@vger.kernel.org>,
Netdev <netdev@vger.kernel.org>,
linux-kernel <linux-kernel@vger.kernel.org>,
Stable <stable@vger.kernel.org>
Subject: Re: [PATCH] carl9170: tx: fix an incorrect use of list iterator
Date: Sun, 27 Mar 2022 23:40:46 +0200 [thread overview]
Message-ID: <CAAd0S9CR=PjEskWAi132qWB5WL1yZwzOBfV-y5m2ERzc3L_qcA@mail.gmail.com> (raw)
In-Reply-To: <20220327072947.10744-1-xiam0nd.tong@gmail.com>
Hi,
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.
Cheers,
Christian
next prev parent reply other threads:[~2022-03-27 21:41 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-03-27 7:29 [PATCH] carl9170: tx: fix an incorrect use of list iterator Xiaomeng Tong
2022-03-27 21:40 ` Christian Lamparter [this message]
2022-03-28 12:25 ` Xiaomeng Tong
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAAd0S9CR=PjEskWAi132qWB5WL1yZwzOBfV-y5m2ERzc3L_qcA@mail.gmail.com' \
--to=chunkeey@gmail.com \
--cc=davem@davemloft.net \
--cc=kuba@kernel.org \
--cc=kvalo@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-wireless@vger.kernel.org \
--cc=linville@tuxdriver.com \
--cc=netdev@vger.kernel.org \
--cc=pabeni@redhat.com \
--cc=stable@vger.kernel.org \
--cc=xiam0nd.tong@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).