2022-01-16 06:45:41

by Pavel Skripkin

[permalink] [raw]
Subject: [PATCH] ath9k_htc: fix uninit value bugs

Syzbot reported 2 KMSAN bugs in ath9k. All of them are caused by missing
field initialization.

In htc_connect_service() svc_meta_len and pad are not initialized. Based
on code it looks like in current skb there is no service data, so simply
initialize svc_meta_len to 0.

htc_issue_send() does not initialize htc_frame_hdr::control array. Based
on firmware code, it will initialize it by itself, so simply zero whole
array to make KMSAN happy

Fail logs:

BUG: KMSAN: kernel-usb-infoleak in usb_submit_urb+0x6c1/0x2aa0 drivers/usb/core/urb.c:430
usb_submit_urb+0x6c1/0x2aa0 drivers/usb/core/urb.c:430
hif_usb_send_regout drivers/net/wireless/ath/ath9k/hif_usb.c:127 [inline]
hif_usb_send+0x5f0/0x16f0 drivers/net/wireless/ath/ath9k/hif_usb.c:479
htc_issue_send drivers/net/wireless/ath/ath9k/htc_hst.c:34 [inline]
htc_connect_service+0x143e/0x1960 drivers/net/wireless/ath/ath9k/htc_hst.c:275
...

Uninit was created at:
slab_post_alloc_hook mm/slab.h:524 [inline]
slab_alloc_node mm/slub.c:3251 [inline]
__kmalloc_node_track_caller+0xe0c/0x1510 mm/slub.c:4974
kmalloc_reserve net/core/skbuff.c:354 [inline]
__alloc_skb+0x545/0xf90 net/core/skbuff.c:426
alloc_skb include/linux/skbuff.h:1126 [inline]
htc_connect_service+0x1029/0x1960 drivers/net/wireless/ath/ath9k/htc_hst.c:258
...

Bytes 4-7 of 18 are uninitialized
Memory access of size 18 starts at ffff888027377e00

BUG: KMSAN: kernel-usb-infoleak in usb_submit_urb+0x6c1/0x2aa0 drivers/usb/core/urb.c:430
usb_submit_urb+0x6c1/0x2aa0 drivers/usb/core/urb.c:430
hif_usb_send_regout drivers/net/wireless/ath/ath9k/hif_usb.c:127 [inline]
hif_usb_send+0x5f0/0x16f0 drivers/net/wireless/ath/ath9k/hif_usb.c:479
htc_issue_send drivers/net/wireless/ath/ath9k/htc_hst.c:34 [inline]
htc_connect_service+0x143e/0x1960 drivers/net/wireless/ath/ath9k/htc_hst.c:275
...

Uninit was created at:
slab_post_alloc_hook mm/slab.h:524 [inline]
slab_alloc_node mm/slub.c:3251 [inline]
__kmalloc_node_track_caller+0xe0c/0x1510 mm/slub.c:4974
kmalloc_reserve net/core/skbuff.c:354 [inline]
__alloc_skb+0x545/0xf90 net/core/skbuff.c:426
alloc_skb include/linux/skbuff.h:1126 [inline]
htc_connect_service+0x1029/0x1960 drivers/net/wireless/ath/ath9k/htc_hst.c:258
...

Bytes 16-17 of 18 are uninitialized
Memory access of size 18 starts at ffff888027377e00

Fixes: fb9987d0f748 ("ath9k_htc: Support for AR9271 chipset.")
Reported-by: [email protected]
Signed-off-by: Pavel Skripkin <[email protected]>
---
drivers/net/wireless/ath/ath9k/htc_hst.c | 5 +++++
1 file changed, 5 insertions(+)

diff --git a/drivers/net/wireless/ath/ath9k/htc_hst.c b/drivers/net/wireless/ath/ath9k/htc_hst.c
index 510e61e97dbc..994ec48b2f66 100644
--- a/drivers/net/wireless/ath/ath9k/htc_hst.c
+++ b/drivers/net/wireless/ath/ath9k/htc_hst.c
@@ -30,6 +30,7 @@ static int htc_issue_send(struct htc_target *target, struct sk_buff* skb,
hdr->endpoint_id = epid;
hdr->flags = flags;
hdr->payload_len = cpu_to_be16(len);
+ memset(hdr->control, 0, sizeof(hdr->control));

status = target->hif->send(target->hif_dev, endpoint->ul_pipeid, skb);

@@ -272,6 +273,10 @@ int htc_connect_service(struct htc_target *target,
conn_msg->dl_pipeid = endpoint->dl_pipeid;
conn_msg->ul_pipeid = endpoint->ul_pipeid;

+ /* To prevent infoleak */
+ conn_msg->svc_meta_len = 0;
+ conn_msg->pad = 0;
+
ret = htc_issue_send(target, skb, skb->len, 0, ENDPOINT0);
if (ret)
goto err;
--
2.34.1


2022-01-18 02:25:48

by Kalle Valo

[permalink] [raw]
Subject: Re: [PATCH] ath9k_htc: fix uninit value bugs

Pavel Skripkin <[email protected]> wrote:

> Syzbot reported 2 KMSAN bugs in ath9k. All of them are caused by missing
> field initialization.
>
> In htc_connect_service() svc_meta_len and pad are not initialized. Based
> on code it looks like in current skb there is no service data, so simply
> initialize svc_meta_len to 0.
>
> htc_issue_send() does not initialize htc_frame_hdr::control array. Based
> on firmware code, it will initialize it by itself, so simply zero whole
> array to make KMSAN happy
>
> Fail logs:
>
> BUG: KMSAN: kernel-usb-infoleak in usb_submit_urb+0x6c1/0x2aa0 drivers/usb/core/urb.c:430
> usb_submit_urb+0x6c1/0x2aa0 drivers/usb/core/urb.c:430
> hif_usb_send_regout drivers/net/wireless/ath/ath9k/hif_usb.c:127 [inline]
> hif_usb_send+0x5f0/0x16f0 drivers/net/wireless/ath/ath9k/hif_usb.c:479
> htc_issue_send drivers/net/wireless/ath/ath9k/htc_hst.c:34 [inline]
> htc_connect_service+0x143e/0x1960 drivers/net/wireless/ath/ath9k/htc_hst.c:275
> ...
>
> Uninit was created at:
> slab_post_alloc_hook mm/slab.h:524 [inline]
> slab_alloc_node mm/slub.c:3251 [inline]
> __kmalloc_node_track_caller+0xe0c/0x1510 mm/slub.c:4974
> kmalloc_reserve net/core/skbuff.c:354 [inline]
> __alloc_skb+0x545/0xf90 net/core/skbuff.c:426
> alloc_skb include/linux/skbuff.h:1126 [inline]
> htc_connect_service+0x1029/0x1960 drivers/net/wireless/ath/ath9k/htc_hst.c:258
> ...
>
> Bytes 4-7 of 18 are uninitialized
> Memory access of size 18 starts at ffff888027377e00
>
> BUG: KMSAN: kernel-usb-infoleak in usb_submit_urb+0x6c1/0x2aa0 drivers/usb/core/urb.c:430
> usb_submit_urb+0x6c1/0x2aa0 drivers/usb/core/urb.c:430
> hif_usb_send_regout drivers/net/wireless/ath/ath9k/hif_usb.c:127 [inline]
> hif_usb_send+0x5f0/0x16f0 drivers/net/wireless/ath/ath9k/hif_usb.c:479
> htc_issue_send drivers/net/wireless/ath/ath9k/htc_hst.c:34 [inline]
> htc_connect_service+0x143e/0x1960 drivers/net/wireless/ath/ath9k/htc_hst.c:275
> ...
>
> Uninit was created at:
> slab_post_alloc_hook mm/slab.h:524 [inline]
> slab_alloc_node mm/slub.c:3251 [inline]
> __kmalloc_node_track_caller+0xe0c/0x1510 mm/slub.c:4974
> kmalloc_reserve net/core/skbuff.c:354 [inline]
> __alloc_skb+0x545/0xf90 net/core/skbuff.c:426
> alloc_skb include/linux/skbuff.h:1126 [inline]
> htc_connect_service+0x1029/0x1960 drivers/net/wireless/ath/ath9k/htc_hst.c:258
> ...
>
> Bytes 16-17 of 18 are uninitialized
> Memory access of size 18 starts at ffff888027377e00
>
> Fixes: fb9987d0f748 ("ath9k_htc: Support for AR9271 chipset.")
> Reported-by: [email protected]
> Signed-off-by: Pavel Skripkin <[email protected]>

How did you test this? As syzbot is mentioned I assume you did not test this on
a real device, it would help a lot if this is clearly mentioned in the commit
log. My trust on syzbot fixes is close to zero due to bad past history.

--
https://patchwork.kernel.org/project/linux-wireless/patch/[email protected]/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

2022-01-31 11:00:53

by Kalle Valo

[permalink] [raw]
Subject: Re: [PATCH] ath9k_htc: fix uninit value bugs

Pavel Skripkin <[email protected]> wrote:

> Syzbot reported 2 KMSAN bugs in ath9k. All of them are caused by missing
> field initialization.
>
> In htc_connect_service() svc_meta_len and pad are not initialized. Based
> on code it looks like in current skb there is no service data, so simply
> initialize svc_meta_len to 0.
>
> htc_issue_send() does not initialize htc_frame_hdr::control array. Based
> on firmware code, it will initialize it by itself, so simply zero whole
> array to make KMSAN happy
>
> Fail logs:
>
> BUG: KMSAN: kernel-usb-infoleak in usb_submit_urb+0x6c1/0x2aa0 drivers/usb/core/urb.c:430
> usb_submit_urb+0x6c1/0x2aa0 drivers/usb/core/urb.c:430
> hif_usb_send_regout drivers/net/wireless/ath/ath9k/hif_usb.c:127 [inline]
> hif_usb_send+0x5f0/0x16f0 drivers/net/wireless/ath/ath9k/hif_usb.c:479
> htc_issue_send drivers/net/wireless/ath/ath9k/htc_hst.c:34 [inline]
> htc_connect_service+0x143e/0x1960 drivers/net/wireless/ath/ath9k/htc_hst.c:275
> ...
>
> Uninit was created at:
> slab_post_alloc_hook mm/slab.h:524 [inline]
> slab_alloc_node mm/slub.c:3251 [inline]
> __kmalloc_node_track_caller+0xe0c/0x1510 mm/slub.c:4974
> kmalloc_reserve net/core/skbuff.c:354 [inline]
> __alloc_skb+0x545/0xf90 net/core/skbuff.c:426
> alloc_skb include/linux/skbuff.h:1126 [inline]
> htc_connect_service+0x1029/0x1960 drivers/net/wireless/ath/ath9k/htc_hst.c:258
> ...
>
> Bytes 4-7 of 18 are uninitialized
> Memory access of size 18 starts at ffff888027377e00
>
> BUG: KMSAN: kernel-usb-infoleak in usb_submit_urb+0x6c1/0x2aa0 drivers/usb/core/urb.c:430
> usb_submit_urb+0x6c1/0x2aa0 drivers/usb/core/urb.c:430
> hif_usb_send_regout drivers/net/wireless/ath/ath9k/hif_usb.c:127 [inline]
> hif_usb_send+0x5f0/0x16f0 drivers/net/wireless/ath/ath9k/hif_usb.c:479
> htc_issue_send drivers/net/wireless/ath/ath9k/htc_hst.c:34 [inline]
> htc_connect_service+0x143e/0x1960 drivers/net/wireless/ath/ath9k/htc_hst.c:275
> ...
>
> Uninit was created at:
> slab_post_alloc_hook mm/slab.h:524 [inline]
> slab_alloc_node mm/slub.c:3251 [inline]
> __kmalloc_node_track_caller+0xe0c/0x1510 mm/slub.c:4974
> kmalloc_reserve net/core/skbuff.c:354 [inline]
> __alloc_skb+0x545/0xf90 net/core/skbuff.c:426
> alloc_skb include/linux/skbuff.h:1126 [inline]
> htc_connect_service+0x1029/0x1960 drivers/net/wireless/ath/ath9k/htc_hst.c:258
> ...
>
> Bytes 16-17 of 18 are uninitialized
> Memory access of size 18 starts at ffff888027377e00
>
> Fixes: fb9987d0f748 ("ath9k_htc: Support for AR9271 chipset.")
> Reported-by: [email protected]
> Signed-off-by: Pavel Skripkin <[email protected]>
> Signed-off-by: Kalle Valo <[email protected]>

Patch applied to ath-next branch of ath.git, thanks.

d1e0df1c57bd ath9k_htc: fix uninit value bugs

--
https://patchwork.kernel.org/project/linux-wireless/patch/[email protected]/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

2022-01-31 11:38:04

by Pavel Skripkin

[permalink] [raw]
Subject: Re: [PATCH] ath9k_htc: fix uninit value bugs

Hi Kalle,

On 1/28/22 15:32, Kalle Valo wrote:
>> Fixes: fb9987d0f748 ("ath9k_htc: Support for AR9271 chipset.")
>> Reported-by: [email protected]
>> Signed-off-by: Pavel Skripkin <[email protected]>
>> Signed-off-by: Kalle Valo <[email protected]>
>
> Patch applied to ath-next branch of ath.git, thanks.
>
> d1e0df1c57bd ath9k_htc: fix uninit value bugs
>

Thanks, Kalle! Can you also, please, check out this one too :)
Quite old, but syzbot is getting mad with this bug (like 20k hits). Thanks!


https://lore.kernel.org/all/[email protected]/




With regards,
Pavel Skripkin