| Message ID | cd4287be-5434-039e-59ba-2a9cb2ab5185@I-love.SAKURA.ne.jp (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Toke Høiland-Jørgensen |
| Headers | show |
| Series | ath9k: avoid uninit memory read in ath9k_htc_rx_msg() | expand |
The "+ 32" part in __dev_alloc_skb(pkt_len + 32, GFP_ATOMIC) is there since
this code was added by commit fb9987d0f748c983 ("ath9k_htc: Support for
AR9271 chipset."). What is the intent of this "+ 32" ?
If this is a safeguard buffer in case pkt_len was invalid,
can we initialize with 0 ?
On 2022/07/30 21:13, Tetsuo Handa 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.
>
> 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>
> ---
> Since I can't find details on possible packet length used by this protocol,
> there might be shorter-but-valid packets. Please check carefully.
>
> drivers/net/wireless/ath/ath9k/htc_hst.c | 43 +++++++++++++++---------
> 1 file changed, 28 insertions(+), 15 deletions(-)
>
> 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:
On 2022/07/30 21:13, Tetsuo Handa wrote: > 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. Which choice do we want to go?
Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> writes: > On 2022/07/30 21:13, Tetsuo Handa wrote: >> 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. > > Which choice do we want to go? I prefer the explicit length checks as you do in your patch. Could you please resend with an updated commit message making it explicit that this is the choice this patch is going with? -Toke
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. 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> --- Since I can't find details on possible packet length used by this protocol, there might be shorter-but-valid packets. Please check carefully. drivers/net/wireless/ath/ath9k/htc_hst.c | 43 +++++++++++++++--------- 1 file changed, 28 insertions(+), 15 deletions(-)