wifi: ath9k: avoid uninit memory read in ath9k_htc_rx_msg()

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
Reported-by: syzbot <[email protected]>
Signed-off-by: Tetsuo Handa <[email protected]>
Acked-by: Toke Høiland-Jørgensen <[email protected]>
Signed-off-by: Kalle Valo <[email protected]>
Link: https://lore.kernel.org/r/[email protected]

diff --git a/drivers/net/wireless/ath/ath9k/htc_hst.c b/drivers/net/wireless/ath/ath9k/htc_hst.c
index 994ec48b2f669588dbfe46c69611aad47aaebfbe..ca05b07a45e67c714e58ac5f9c708acfb1b8f1c8 100644 (file)
--- a/drivers/net/wireless/ath/ath9k/htc_hst.c
+++ b/drivers/net/wireless/ath/ath9k/htc_hst.c
@@ -364,33 +364,27 @@ ret:
 }
 
 static void ath9k_htc_fw_panic_report(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;
 
 {
        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);
                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);
                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;
 
        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) {
        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) {
                kfree_skb(skb);
                return;
        }
 
        if (epid < 0 || epid >= ENDPOINT_MAX) {

+invalid:

                if (pipe_id != USB_REG_IN_PIPE)
                        dev_kfree_skb_any(skb);
                else
                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) {
 
                /* 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);
                                /* Move past the Watchdog pattern */
                                htc_hdr = (struct htc_frame_hdr *)(skb->data + 4);

+                               len -= 4;
+                       }

                }
 
                /* Get the message ID */
                }
 
                /* 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:
                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:
                        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:
                        htc_process_conn_rsp(htc_handle, htc_hdr);
                        break;
                default: