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@xxxxxxxxxxxxxxxxxxxxxxxxx>
>
> Signed-off-by: Tetsuo Handa <penguin-kernel@xxxxxxxxxxxxxxxxxxx>
> ---
> 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:
