| Message ID | 7acfa1be-4b5c-b2ce-de43-95b0593fb3e5@I-love.SAKURA.ne.jp |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] ath9k: avoid uninit memory read in ath9k_htc_rx_msg() | expand |
Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> writes: > syzbot is reporting uninit value at ath9k_htc_rx_msg() [1], for > ioctl(USB_RAW_IOCTL_EP_WRITE) can call ath9k_hif_usb_rx_stream() with > pkt_len = 0 but ath9k_hif_usb_rx_stream() uses > __dev_alloc_skb(pkt_len + 32, GFP_ATOMIC) based on an assumption that > pkt_len is valid. As a result, ath9k_hif_usb_rx_stream() allocates skb > with uninitialized memory and ath9k_htc_rx_msg() is reading from > uninitialized memory. > > Since bytes accessed by ath9k_htc_rx_msg() is not known until > ath9k_htc_rx_msg() is called, it would be difficult to check minimal valid > pkt_len at "if (pkt_len > 2 * MAX_RX_BUF_SIZE) {" line in > ath9k_hif_usb_rx_stream(). > > We have two choices. One is to workaround by adding __GFP_ZERO so that > ath9k_htc_rx_msg() sees 0 if pkt_len is invalid. The other is to let > ath9k_htc_rx_msg() validate pkt_len before accessing. This patch chose > the latter. > > Note that I'm not sure threshold condition is correct, for I can't find > details on possible packet length used by this protocol. > > Link: https://syzkaller.appspot.com/bug?extid=2ca247c2d60c7023de7f [1] > Reported-by: syzbot <syzbot+2ca247c2d60c7023de7f@syzkaller.appspotmail.com> > Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> Acked-by: Toke Høiland-Jørgensen <toke@toke.dk>
Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> wrote: > syzbot is reporting uninit value at ath9k_htc_rx_msg() [1], for > ioctl(USB_RAW_IOCTL_EP_WRITE) can call ath9k_hif_usb_rx_stream() with > pkt_len = 0 but ath9k_hif_usb_rx_stream() uses > __dev_alloc_skb(pkt_len + 32, GFP_ATOMIC) based on an assumption that > pkt_len is valid. As a result, ath9k_hif_usb_rx_stream() allocates skb > with uninitialized memory and ath9k_htc_rx_msg() is reading from > uninitialized memory. > > Since bytes accessed by ath9k_htc_rx_msg() is not known until > ath9k_htc_rx_msg() is called, it would be difficult to check minimal valid > pkt_len at "if (pkt_len > 2 * MAX_RX_BUF_SIZE) {" line in > ath9k_hif_usb_rx_stream(). > > We have two choices. One is to workaround by adding __GFP_ZERO so that > ath9k_htc_rx_msg() sees 0 if pkt_len is invalid. The other is to let > ath9k_htc_rx_msg() validate pkt_len before accessing. This patch chose > the latter. > > Note that I'm not sure threshold condition is correct, for I can't find > details on possible packet length used by this protocol. > > Link: https://syzkaller.appspot.com/bug?extid=2ca247c2d60c7023de7f [1] > Reported-by: syzbot <syzbot+2ca247c2d60c7023de7f@syzkaller.appspotmail.com> > Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> > Acked-by: Toke Høiland-Jørgensen <toke@toke.dk> > Signed-off-by: Kalle Valo <quic_kvalo@quicinc.com> Patch applied to ath-next branch of ath.git, thanks. b383e8abed41 wifi: ath9k: avoid uninit memory read in ath9k_htc_rx_msg()
diff --git a/drivers/net/wireless/ath/ath9k/htc_hst.c b/drivers/net/wireless/ath/ath9k/htc_hst.c index 994ec48b2f66..ca05b07a45e6 100644 --- a/drivers/net/wireless/ath/ath9k/htc_hst.c +++ b/drivers/net/wireless/ath/ath9k/htc_hst.c @@ -364,33 +364,27 @@ void ath9k_htc_txcompletion_cb(struct htc_target *htc_handle, } static void ath9k_htc_fw_panic_report(struct htc_target *htc_handle, - struct sk_buff *skb) + struct sk_buff *skb, u32 len) { uint32_t *pattern = (uint32_t *)skb->data; - switch (*pattern) { - case 0x33221199: - { + if (*pattern == 0x33221199 && len >= sizeof(struct htc_panic_bad_vaddr)) { struct htc_panic_bad_vaddr *htc_panic; htc_panic = (struct htc_panic_bad_vaddr *) skb->data; dev_err(htc_handle->dev, "ath: firmware panic! " "exccause: 0x%08x; pc: 0x%08x; badvaddr: 0x%08x.\n", htc_panic->exccause, htc_panic->pc, htc_panic->badvaddr); - break; - } - case 0x33221299: - { + return; + } + if (*pattern == 0x33221299) { struct htc_panic_bad_epid *htc_panic; htc_panic = (struct htc_panic_bad_epid *) skb->data; dev_err(htc_handle->dev, "ath: firmware panic! " "bad epid: 0x%08x\n", htc_panic->epid); - break; - } - default: - dev_err(htc_handle->dev, "ath: unknown panic pattern!\n"); - break; + return; } + dev_err(htc_handle->dev, "ath: unknown panic pattern!\n"); } /* @@ -411,16 +405,26 @@ void ath9k_htc_rx_msg(struct htc_target *htc_handle, if (!htc_handle || !skb) return; + /* A valid message requires len >= 8. + * + * sizeof(struct htc_frame_hdr) == 8 + * sizeof(struct htc_ready_msg) == 8 + * sizeof(struct htc_panic_bad_vaddr) == 16 + * sizeof(struct htc_panic_bad_epid) == 8 + */ + if (unlikely(len < sizeof(struct htc_frame_hdr))) + goto invalid; htc_hdr = (struct htc_frame_hdr *) skb->data; epid = htc_hdr->endpoint_id; if (epid == 0x99) { - ath9k_htc_fw_panic_report(htc_handle, skb); + ath9k_htc_fw_panic_report(htc_handle, skb, len); kfree_skb(skb); return; } if (epid < 0 || epid >= ENDPOINT_MAX) { +invalid: if (pipe_id != USB_REG_IN_PIPE) dev_kfree_skb_any(skb); else @@ -432,21 +436,30 @@ void ath9k_htc_rx_msg(struct htc_target *htc_handle, /* Handle trailer */ if (htc_hdr->flags & HTC_FLAGS_RECV_TRAILER) { - if (be32_to_cpu(*(__be32 *) skb->data) == 0x00C60000) + if (be32_to_cpu(*(__be32 *) skb->data) == 0x00C60000) { /* Move past the Watchdog pattern */ htc_hdr = (struct htc_frame_hdr *)(skb->data + 4); + len -= 4; + } } /* Get the message ID */ + if (unlikely(len < sizeof(struct htc_frame_hdr) + sizeof(__be16))) + goto invalid; msg_id = (__be16 *) ((void *) htc_hdr + sizeof(struct htc_frame_hdr)); /* Now process HTC messages */ switch (be16_to_cpu(*msg_id)) { case HTC_MSG_READY_ID: + if (unlikely(len < sizeof(struct htc_ready_msg))) + goto invalid; htc_process_target_rdy(htc_handle, htc_hdr); break; case HTC_MSG_CONNECT_SERVICE_RESPONSE_ID: + if (unlikely(len < sizeof(struct htc_frame_hdr) + + sizeof(struct htc_conn_svc_rspmsg))) + goto invalid; htc_process_conn_rsp(htc_handle, htc_hdr); break; default:
syzbot is reporting uninit value at ath9k_htc_rx_msg() [1], for ioctl(USB_RAW_IOCTL_EP_WRITE) can call ath9k_hif_usb_rx_stream() with pkt_len = 0 but ath9k_hif_usb_rx_stream() uses __dev_alloc_skb(pkt_len + 32, GFP_ATOMIC) based on an assumption that pkt_len is valid. As a result, ath9k_hif_usb_rx_stream() allocates skb with uninitialized memory and ath9k_htc_rx_msg() is reading from uninitialized memory. Since bytes accessed by ath9k_htc_rx_msg() is not known until ath9k_htc_rx_msg() is called, it would be difficult to check minimal valid pkt_len at "if (pkt_len > 2 * MAX_RX_BUF_SIZE) {" line in ath9k_hif_usb_rx_stream(). We have two choices. One is to workaround by adding __GFP_ZERO so that ath9k_htc_rx_msg() sees 0 if pkt_len is invalid. The other is to let ath9k_htc_rx_msg() validate pkt_len before accessing. This patch chose the latter. Note that I'm not sure threshold condition is correct, for I can't find details on possible packet length used by this protocol. Link: https://syzkaller.appspot.com/bug?extid=2ca247c2d60c7023de7f [1] Reported-by: syzbot <syzbot+2ca247c2d60c7023de7f@syzkaller.appspotmail.com> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> --- Changes in v2: Update patch description. drivers/net/wireless/ath/ath9k/htc_hst.c | 43 +++++++++++++++--------- 1 file changed, 28 insertions(+), 15 deletions(-)