netdev - Re: [PATCH v1 1/2] ath9k: fix ath_get_rate_txpower() to respect the rate list end tag

lists.openwall.net		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 linux-cve-announce 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]

Message-ID: <87v8vncpvo.fsf@toke.dk>
Date:   Tue, 05 Apr 2022 21:05:31 +0200
From:   Toke Høiland-Jørgensen <toke@...e.dk>
To:     Peter Seiderer <ps.report@....net>
Cc:     linux-wireless@...r.kernel.org, Kalle Valo <kvalo@...nel.org>,
        "David S . Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Johannes Berg <johannes@...solutions.net>,
        netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1 1/2] ath9k: fix ath_get_rate_txpower() to respect the
 rate list end tag

Peter Seiderer <ps.report@....net> writes:

> Hello Toke,
>
> On Mon, 04 Apr 2022 20:19:39 +0200, Toke Høiland-Jørgensen <toke@...e.dk> wrote:
>
>> Peter Seiderer <ps.report@....net> writes:
>> 
>> > Stop reading (and copying) from ieee80211_tx_rate to ath_tx_info.rates
>> > after list end tag (count == 0, idx < 0), prevents copying of garbage
>> > to card registers.  
>> 
>> In the normal case I don't think this patch does anything, since any
>> invalid rate entries will already be skipped (just one at a time instead
>> of all at once). So this comment is a bit misleading.
>
> Save some (minimal) compute time? Found it something misleading while
> debugging to see random values written out to the card and found this
> comment in net/mac80211/rate.c:
>
>  648                 /*
>  649                  * make sure there's no valid rate following
>  650                  * an invalid one, just in case drivers don't
>  651                  * take the API seriously to stop at -1.
>  652                  */
>
> and multiple places doing the same check (count == 0, idx < 0) for validation
> e.g.:
>
>  723                 if (i < ARRAY_SIZE(info->control.rates) &&
>  724                     info->control.rates[i].idx >= 0 &&
>  725                     info->control.rates[i].count) {
>
> or 
>
>  742                 if (rates[i].idx < 0 || !rates[i].count)
>  743                         break;
>
>> 
>> Also, Minstrel could in principle produce a rate sequence where the
>> indexes are all positive, but there's one in the middle with a count of
>> 0, couldn't it? With this patch, the last entries of such a sequence
>> would now be skipped...
>
> According to net/mac80211/rc80211_minstrel_ht.c:
>
> 1128 static bool
> 1129 minstrel_ht_txstat_valid(struct minstrel_priv *mp, struct minstrel_ht_sta *     mi,
> 1130                          struct ieee80211_tx_rate *rate)
> 1131 {
> 1132         int i;
> 1133 
> 1134         if (rate->idx < 0)
> 1135                 return false;
> 1136 
> 1137         if (!rate->count)
> 1138                 return false;
> 1139 
>
> minstrel although evaluates a rate count of zero as invalid...

So my concern was mostly that the documentation (in mac80211.h) says
that an idx of -1 indicates the end, but says nothing about the count.
Which implies that in principle you could have a rate table of { idx,
count } like { 1, 1 }, { 2, 0 }, { 3, 1 } which would mean all three
rates was valid but the second one would just be "skipped" due to a
count of zero.

But it seems that the code populating the rate table that you linked
above (lines 742/743) actually do abort on either condition, so I guess
it's safe to do so in the driver as well...

-Toke