Re: [PATCH] wifi: carl9170: fix stack-out-of-bounds in carl9170_cmd_callback

[Christian Lamparter <chunkeey@gmail.com>]

Hi,

On 4/24/26 5:17 AM, Deepanshu Kartikey wrote:
> carl9170_cmd_callback() does not return after calling
> carl9170_restart() when an invalid command response is detected.
> This causes a fall-through into the memcpy block below, where
> ar->readbuf is written with a device-controlled length (len - 4)
> instead of the expected ar->readlen bytes.
> 
> A malicious or fuzzing USB device can send an oversized response
> (e.g. 60 bytes) causing a stack-out-of-bounds write into ar->readbuf,
> as detected by KASAN.
> 
> Fix this by adding a return after carl9170_restart() to match the
> original intent stated in the comment ("Do not complete"). Also cap
> the memcpy with min_t() as defense-in-depth to prevent overflow even
> if the control flow changes in future.
> 
> The bug has been present since the initial driver submission in 2010.


I've seen this before in a mail from Tristan Madani (CC'd) on the 13th of April 2026.
Unfortunately, he didn't post this to the linux-wireless mailing-list. Instead
he went for the Security Officers <security@kernel.org> , so I can't provide any link to it.
That said, he since changed it because of the notes I had about the driver actually want to process
further so only the memcpy that caused the overflow was modified with the same
"min_t(u32, len - 4, ar->readlen)" instead of "len -4" as the length:
"[PATCH v3 1/3] wifi: carl9170: bound memcpy length in cmd callback to prevent OOB read"

https://lore.kernel.org/linux-wireless/20260421134929.325662-2-tristmd@gmail.com/

So, who should get the credit? Maybe another person will post a patch soon too? Who knows.

Note: if you used any assistents. Please also check out the (new) guide-lines:
https://docs.kernel.org/process/coding-assistants.html and give proper attributions
to the used tools.

> Fixes: a84fab3cbfdc ("carl9170: 802.11 rx/tx processing and usb backend")
> Reported-by: syzbot+5c1ca6ccaa1215781cac@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=5c1ca6ccaa1215781cac


> Tested-by: syzbot+5c1ca6ccaa1215781cac@syzkaller.appspotmail.com
> Signed-off-by: Deepanshu Kartikey <kartikey406@gmail.com>
> ---
>   drivers/net/wireless/ath/carl9170/rx.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/wireless/ath/carl9170/rx.c b/drivers/net/wireless/ath/carl9170/rx.c
> index 6833430130f4..6a5923495a01 100644
> --- a/drivers/net/wireless/ath/carl9170/rx.c
> +++ b/drivers/net/wireless/ath/carl9170/rx.c
> @@ -145,12 +145,14 @@ static void carl9170_cmd_callback(struct ar9170 *ar, u32 len, void *buffer)
>   		 * and we get a stack trace from there.
>   		 */
>   		carl9170_restart(ar, CARL9170_RR_INVALID_RSP);
> +		return;

It does happen that the driver receives garbage from the device. To remain bug-to-bug compatible with
the rest of the driver should just handle it gracefully. (It's more like the comment above the
carl9170_restart outdated and could be removed. Because carl9170_restart can start an async process that
nukes the whole driver with a full usb_reset which will unbind the device... And if its too far gone it
won't be able to rebind - then it requires the user to either physically unplugged and reinserted the
wifi stick or reboot (only if it helps) or shutdown the machine)

>   	}
>   
>   	spin_lock(&ar->cmd_lock);
>   	if (ar->readbuf) {
>   		if (len >= 4)
> -			memcpy(ar->readbuf, buffer + 4, len - 4);
> +			memcpy(ar->readbuf, buffer + 4,
> +			       min_t(u32, len - 4, ar->readlen));

yes.

>   
>   		ar->readbuf = NULL;
>   	}


Cheers,
Christian