lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  PHC 
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Sun, 27 Mar 2022 23:40:46 +0200
From:   Christian Lamparter <>
To:     Xiaomeng Tong <>
Cc:     Kalle Valo <>, David Miller <>,
        Jakub Kicinski <>,,
        "John W. Linville" <>,
        linux-wireless <>,
        Netdev <>,
        linux-kernel <>,
        Stable <>
Subject: Re: [PATCH] carl9170: tx: fix an incorrect use of list iterator


On Sun, Mar 27, 2022 at 8:10 PM Xiaomeng Tong <> 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:
> Fixes: 1f1d9654e183c ("carl9170: refactor carl9170_update_beacon")
> Signed-off-by: Xiaomeng Tong <>
> ---
>  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.


Powered by blists - more mailing lists