Re: rtw88: 8822CE passes corrupted A-MSDUs as hw-decrypted, poisoning CCMP replay counter

[Matthew Leach <matthew.leach@collabora.com>]

"Luka Gejak" <luka.gejak@linux.dev> writes:

> Hi Bob,

Hi Luka,

> Thanks for the write up. I have a different read on this one. I don't
> think those frames were decrypted at all, and I think the descriptor
> is telling us less than it looks. Notes below.

Thanks for taking the time to look at this.

>> the bytes from the CCMP header onwards are garbage.
>
> That data is not from this frame. Your jump 2 dump shows where it
> comes from. The drv_info area is the 32 bytes at offset 24, and the
> hardware leaves it alone when PHYST is 0, so whatever is siting there
> is old. At offset 32 there is a whole RX descriptor, same w0, w1, w3
> and w4 as the live one, w2 sequence number 0xc5d and TSF 0x0042a3fa.
> The live frame is sequence 0xc66 with TSF 0x0042a4f3. So that memory
> is from ~9 frames and ~250us earlier.
>
> The ring has 512 buffers and one frame per buffer, so the same buffer
> only comes around again after about 512 frames. Data from approx. 9
> frames ago can't be leftover from the last time that buffer was used,
> so I think the hardware wrote it for some other frame. So for the
> frame we got, the hardware wrote the descriptor and the 802.11 header
> and then stopped, and the rest of the buffer still had that other
> frame in it.
>
> Same for the feame 3. Its first four CCMP bytes are 44(PN0) 50(PN1)
> 00(the zero reserved byte) 20 (ExtIV), so that write got four bytes in
> the CCMP header and then stopped.

The fresh header followed by unexpected contents is consistent with an
incomplete write, although I don't think we can distinguish that from
consuming the buffer before all payload writes are visible. Could we
compare the RX buffer contents when completion is first observed and
again before returning the buffer to the hardware? If the contents
change while the buffer is still host-owned, that would support
premature completion signalling.

>> our reading is that for these frames the payload/decrypt path produced
>> wrong data while the descriptor still reported CRC OK, ICV OK and
>> hardware decrypted.
>
> I read it the other way. Nothing was decrypted into that area. The CRC, ICV
> and decrypt bits are the results of checks the hardware does before it hands
> the frame over,

My understanding is that BIT_AICV in RCR controls acceptance of
integrity-error frames, and it is unset in our register snapshots.
Hardware should therefore already be filtering those frames.

That doesn't rule out the scenario you describe, where the frame passes
the integrity check internally but the bytes delivered to host memory
are wrong. However, all four captured bad frames have ICV_ERR clear, so
checking that bit alone would not catch these examples.

> and the payload write is a separate step after those checks which just
> did not happen here. So a good descriptor doesn't mean the frame in
> the buffer is real.
>
> My guess for what takes the link down is that rtw_rx_query_rx_desc sets
> decrypted from swdec and the enc type alone, without looking at icv_err, and
> then rtw_rx_fill_rx_status sets RX_FLAG_DECRYPTED, with which mac80211
> reads the PN out of the frame itself and saves it as the last PN for the
> key, and it only rejects a PN that is lower. So one frame like this pins the
> key to a random PN, and everything after it is dropped as a replay until new
> keys go in.

Yes, this is our take. Somehow the bogus PN gets passed to mac80211.
That then poisons the kernel's own PN value, which causes all subsequent
packets, which have a good PN, to be dropped as replay attacks.

> I will send a patch that fixes this part  so you can test it. It should
> turn a dead link into a dropped frame, but either way the test will
> tell.

Thanks for that patch; I've taken a look. The CCMP-header validation
should catch jumps 1, 2 and 4. However, jump 3 has valid reserved/ExtIV
bytes (44 50 00 20) and corruption in the remaining PN bytes, so this
patch would retain RX_FLAG_DECRYPTED and still poison the replay
counter. This looks useful as a partial mitigation, but doesn’t yet
resolve the reported failure.

Regards,
-- 
Matt