| Message ID | 1585625296-31013-1-git-send-email-hqjagain@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | ath9k: fix stack-out-of-bounds Write in ath9k_hif_usb_rx_cb | expand |
Qiujun Huang <hqjagain@gmail.com> writes: > Add barrier to accessing the stack array skb_pool. > > Reported-by: syzbot+d403396d4df67ad0bd5f@syzkaller.appspotmail.com > Signed-off-by: Qiujun Huang <hqjagain@gmail.com> > --- > drivers/net/wireless/ath/ath9k/hif_usb.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/drivers/net/wireless/ath/ath9k/hif_usb.c b/drivers/net/wireless/ath/ath9k/hif_usb.c > index dd0c323..c4a2b72 100644 > --- a/drivers/net/wireless/ath/ath9k/hif_usb.c > +++ b/drivers/net/wireless/ath/ath9k/hif_usb.c > @@ -612,6 +612,11 @@ static void ath9k_hif_usb_rx_stream(struct hif_device_usb *hif_dev, > hif_dev->remain_skb = nskb; > spin_unlock(&hif_dev->rx_lock); > } else { > + if (pool_index == MAX_PKT_NUM_IN_TRANSFER) { > + dev_err(&hif_dev->udev->dev, > + "ath9k_htc: over RX MAX_PKT_NUM\n"); > + goto err; > + } What about 'pool_index >= MAX_PKT_NUM_IN_TRANSFER' just to be on the safe side? Ah, but then error handling won't work: err: for (i = 0; i < pool_index; i++) { RX_STAT_ADD(skb_completed_bytes, skb_pool[i]->len); ath9k_htc_rx_msg(hif_dev->htc_handle, skb_pool[i], skb_pool[i]->len, USB_WLAN_RX_PIPE); RX_STAT_INC(skb_completed); } Maybe that should use 'min(pool_index, MAX_PKT_NUM_IN_TRANSFER - 1)' or something? Or maybe it's just overengineerin, dunno.
On Tue, Mar 31, 2020 at 7:03 PM Kalle Valo <kvalo@codeaurora.org> wrote: > > Qiujun Huang <hqjagain@gmail.com> writes: > > > Add barrier to accessing the stack array skb_pool. > > > > Reported-by: syzbot+d403396d4df67ad0bd5f@syzkaller.appspotmail.com > > Signed-off-by: Qiujun Huang <hqjagain@gmail.com> > > --- > > drivers/net/wireless/ath/ath9k/hif_usb.c | 5 +++++ > > 1 file changed, 5 insertions(+) > > > > diff --git a/drivers/net/wireless/ath/ath9k/hif_usb.c b/drivers/net/wireless/ath/ath9k/hif_usb.c > > index dd0c323..c4a2b72 100644 > > --- a/drivers/net/wireless/ath/ath9k/hif_usb.c > > +++ b/drivers/net/wireless/ath/ath9k/hif_usb.c > > @@ -612,6 +612,11 @@ static void ath9k_hif_usb_rx_stream(struct hif_device_usb *hif_dev, > > hif_dev->remain_skb = nskb; > > spin_unlock(&hif_dev->rx_lock); > > } else { > > + if (pool_index == MAX_PKT_NUM_IN_TRANSFER) { > > + dev_err(&hif_dev->udev->dev, > > + "ath9k_htc: over RX MAX_PKT_NUM\n"); > > + goto err; > > + } > > What about 'pool_index >= MAX_PKT_NUM_IN_TRANSFER' just to be on the > safe side? Ah, but then error handling won't work: Get that. > > err: > for (i = 0; i < pool_index; i++) { > RX_STAT_ADD(skb_completed_bytes, skb_pool[i]->len); > ath9k_htc_rx_msg(hif_dev->htc_handle, skb_pool[i], > skb_pool[i]->len, USB_WLAN_RX_PIPE); > RX_STAT_INC(skb_completed); > } > > Maybe that should use 'min(pool_index, MAX_PKT_NUM_IN_TRANSFER - 1)' or > something? Or maybe it's just overengineerin, dunno. I will take a deeper look, thanks. > > -- > https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
On Tue, Mar 31, 2020 at 7:03 PM Kalle Valo <kvalo@codeaurora.org> wrote: > > Qiujun Huang <hqjagain@gmail.com> writes: > > > Add barrier to accessing the stack array skb_pool. > > > > Reported-by: syzbot+d403396d4df67ad0bd5f@syzkaller.appspotmail.com > > Signed-off-by: Qiujun Huang <hqjagain@gmail.com> > > --- > > drivers/net/wireless/ath/ath9k/hif_usb.c | 5 +++++ > > 1 file changed, 5 insertions(+) > > > > diff --git a/drivers/net/wireless/ath/ath9k/hif_usb.c b/drivers/net/wireless/ath/ath9k/hif_usb.c > > index dd0c323..c4a2b72 100644 > > --- a/drivers/net/wireless/ath/ath9k/hif_usb.c > > +++ b/drivers/net/wireless/ath/ath9k/hif_usb.c > > @@ -612,6 +612,11 @@ static void ath9k_hif_usb_rx_stream(struct hif_device_usb *hif_dev, > > hif_dev->remain_skb = nskb; > > spin_unlock(&hif_dev->rx_lock); > > } else { > > + if (pool_index == MAX_PKT_NUM_IN_TRANSFER) { > > + dev_err(&hif_dev->udev->dev, > > + "ath9k_htc: over RX MAX_PKT_NUM\n"); > > + goto err; the barrier ensure pool_index <= MAX_PKT_NUM_IN_TRANSFER > > + } > > What about 'pool_index >= MAX_PKT_NUM_IN_TRANSFER' just to be on the > safe side? Ah, but then error handling won't work: It looks ok? it can handle the case: pool_index == MAX_PKT_NUM_IN_TRANSFER > > err: > for (i = 0; i < pool_index; i++) { > RX_STAT_ADD(skb_completed_bytes, skb_pool[i]->len); > ath9k_htc_rx_msg(hif_dev->htc_handle, skb_pool[i], > skb_pool[i]->len, USB_WLAN_RX_PIPE); > RX_STAT_INC(skb_completed); > } > > Maybe that should use 'min(pool_index, MAX_PKT_NUM_IN_TRANSFER - 1)' or > something? Or maybe it's just overengineerin, dunno. > > -- > https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
diff --git a/drivers/net/wireless/ath/ath9k/hif_usb.c b/drivers/net/wireless/ath/ath9k/hif_usb.c index dd0c323..c4a2b72 100644 --- a/drivers/net/wireless/ath/ath9k/hif_usb.c +++ b/drivers/net/wireless/ath/ath9k/hif_usb.c @@ -612,6 +612,11 @@ static void ath9k_hif_usb_rx_stream(struct hif_device_usb *hif_dev, hif_dev->remain_skb = nskb; spin_unlock(&hif_dev->rx_lock); } else { + if (pool_index == MAX_PKT_NUM_IN_TRANSFER) { + dev_err(&hif_dev->udev->dev, + "ath9k_htc: over RX MAX_PKT_NUM\n"); + goto err; + } nskb = __dev_alloc_skb(pkt_len + 32, GFP_ATOMIC); if (!nskb) { dev_err(&hif_dev->udev->dev,
Add barrier to accessing the stack array skb_pool. Reported-by: syzbot+d403396d4df67ad0bd5f@syzkaller.appspotmail.com Signed-off-by: Qiujun Huang <hqjagain@gmail.com> --- drivers/net/wireless/ath/ath9k/hif_usb.c | 5 +++++ 1 file changed, 5 insertions(+)