| Message ID | 20210511071926.8951-1-ihuguet@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | rtl8xxxu: avoid parsing short RX packet | expand |
On Tue, May 11, 2021 at 9:19 AM Íñigo Huguet <ihuguet@redhat.com> wrote: > > One USB data buffer can contain multiple received network > packets. If that's the case, they're processed this way: > 1. Original buffer is cloned > 2. Original buffer is trimmed to contain only the first > network packet > 3. This first network packet is passed to network stack > 4. Cloned buffer is trimmed to eliminate the first network > packet > 5. Repeat with the cloned buffer until there are no more > network packets inside > > However, if the space remaining in original buffer after > the first network packet is not enough to contain at least > another network packet descriptor, it is not cloned. > > The loop parsing this packets ended if remaining space == 0. > But if the remaining space was > 0 but < packet descriptor > size, another iteration of the loop was done, processing again > the previous packet because cloning didn't happen. Moreover, > the ownership of this packet had been passed to network > stack in the previous iteration. > > This patch ensures that no extra iteration is done if the > remaining size is not enough for one packet, and also avoid > the first iteration for the same reason. > > Probably this doesn't happen in practice, but can happen > theoretically. > > Signed-off-by: Íñigo Huguet <ihuguet@redhat.com> > --- > drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c | 11 +++++++++-- > 1 file changed, 9 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c > index 9ff09cf7eb62..673961a82c40 100644 > --- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c > +++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c > @@ -5554,6 +5554,11 @@ int rtl8xxxu_parse_rxdesc16(struct rtl8xxxu_priv *priv, struct sk_buff *skb) > urb_len = skb->len; > pkt_cnt = 0; > > + if (urb_len < sizeof(struct rtl8xxxu_rxdesc16)) { > + kfree_skb(skb); > + return RX_TYPE_ERROR; > + } > + > do { > rx_desc = (struct rtl8xxxu_rxdesc16 *)skb->data; > _rx_desc_le = (__le32 *)skb->data; > @@ -5581,7 +5586,7 @@ int rtl8xxxu_parse_rxdesc16(struct rtl8xxxu_priv *priv, struct sk_buff *skb) > * at least cover the rx descriptor > */ > if (pkt_cnt > 1 && > - urb_len > (pkt_offset + sizeof(struct rtl8xxxu_rxdesc16))) > + urb_len >= (pkt_offset + sizeof(struct rtl8xxxu_rxdesc16))) > next_skb = skb_clone(skb, GFP_ATOMIC); > > rx_status = IEEE80211_SKB_RXCB(skb); > @@ -5627,7 +5632,9 @@ int rtl8xxxu_parse_rxdesc16(struct rtl8xxxu_priv *priv, struct sk_buff *skb) > > pkt_cnt--; > urb_len -= pkt_offset; > - } while (skb && urb_len > 0 && pkt_cnt > 0); > + next_skb = NULL; > + } while (skb && pkt_cnt > 0 && > + urb_len >= sizeof(struct rtl8xxxu_rxdesc16)); > > return RX_TYPE_DATA_PKT; > } > -- > 2.31.1 > Hello, About 3 weeks ago I sent this patch, but received no response. Any feedback would be appreciated. Thanks!
Íñigo Huguet <ihuguet@redhat.com> writes: > On Tue, May 11, 2021 at 9:19 AM Íñigo Huguet <ihuguet@redhat.com> wrote: >> >> One USB data buffer can contain multiple received network >> packets. If that's the case, they're processed this way: >> 1. Original buffer is cloned >> 2. Original buffer is trimmed to contain only the first >> network packet >> 3. This first network packet is passed to network stack >> 4. Cloned buffer is trimmed to eliminate the first network >> packet >> 5. Repeat with the cloned buffer until there are no more >> network packets inside >> >> However, if the space remaining in original buffer after >> the first network packet is not enough to contain at least >> another network packet descriptor, it is not cloned. >> >> The loop parsing this packets ended if remaining space == 0. >> But if the remaining space was > 0 but < packet descriptor >> size, another iteration of the loop was done, processing again >> the previous packet because cloning didn't happen. Moreover, >> the ownership of this packet had been passed to network >> stack in the previous iteration. >> >> This patch ensures that no extra iteration is done if the >> remaining size is not enough for one packet, and also avoid >> the first iteration for the same reason. >> >> Probably this doesn't happen in practice, but can happen >> theoretically. >> >> Signed-off-by: Íñigo Huguet <ihuguet@redhat.com> > > About 3 weeks ago I sent this patch, but received no response. Any > feedback would be appreciated. Maintainers are sometimes so busy that review takes extra long, but you can always check the state in patchwork: https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches#checking_state_of_patches_from_patchwork -- https://patchwork.kernel.org/project/linux-wireless/list/ https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
Íñigo Huguet <ihuguet@redhat.com> wrote: > One USB data buffer can contain multiple received network > packets. If that's the case, they're processed this way: > 1. Original buffer is cloned > 2. Original buffer is trimmed to contain only the first > network packet > 3. This first network packet is passed to network stack > 4. Cloned buffer is trimmed to eliminate the first network > packet > 5. Repeat with the cloned buffer until there are no more > network packets inside > > However, if the space remaining in original buffer after > the first network packet is not enough to contain at least > another network packet descriptor, it is not cloned. > > The loop parsing this packets ended if remaining space == 0. > But if the remaining space was > 0 but < packet descriptor > size, another iteration of the loop was done, processing again > the previous packet because cloning didn't happen. Moreover, > the ownership of this packet had been passed to network > stack in the previous iteration. > > This patch ensures that no extra iteration is done if the > remaining size is not enough for one packet, and also avoid > the first iteration for the same reason. > > Probably this doesn't happen in practice, but can happen > theoretically. > > Signed-off-by: Íñigo Huguet <ihuguet@redhat.com> Patch applied to wireless-drivers-next.git, thanks. adf6a0f8c0a6 rtl8xxxu: avoid parsing short RX packet -- https://patchwork.kernel.org/project/linux-wireless/patch/20210511071926.8951-1-ihuguet@redhat.com/ https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c index 9ff09cf7eb62..673961a82c40 100644 --- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c +++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c @@ -5554,6 +5554,11 @@ int rtl8xxxu_parse_rxdesc16(struct rtl8xxxu_priv *priv, struct sk_buff *skb) urb_len = skb->len; pkt_cnt = 0; + if (urb_len < sizeof(struct rtl8xxxu_rxdesc16)) { + kfree_skb(skb); + return RX_TYPE_ERROR; + } + do { rx_desc = (struct rtl8xxxu_rxdesc16 *)skb->data; _rx_desc_le = (__le32 *)skb->data; @@ -5581,7 +5586,7 @@ int rtl8xxxu_parse_rxdesc16(struct rtl8xxxu_priv *priv, struct sk_buff *skb) * at least cover the rx descriptor */ if (pkt_cnt > 1 && - urb_len > (pkt_offset + sizeof(struct rtl8xxxu_rxdesc16))) + urb_len >= (pkt_offset + sizeof(struct rtl8xxxu_rxdesc16))) next_skb = skb_clone(skb, GFP_ATOMIC); rx_status = IEEE80211_SKB_RXCB(skb); @@ -5627,7 +5632,9 @@ int rtl8xxxu_parse_rxdesc16(struct rtl8xxxu_priv *priv, struct sk_buff *skb) pkt_cnt--; urb_len -= pkt_offset; - } while (skb && urb_len > 0 && pkt_cnt > 0); + next_skb = NULL; + } while (skb && pkt_cnt > 0 && + urb_len >= sizeof(struct rtl8xxxu_rxdesc16)); return RX_TYPE_DATA_PKT; }
One USB data buffer can contain multiple received network packets. If that's the case, they're processed this way: 1. Original buffer is cloned 2. Original buffer is trimmed to contain only the first network packet 3. This first network packet is passed to network stack 4. Cloned buffer is trimmed to eliminate the first network packet 5. Repeat with the cloned buffer until there are no more network packets inside However, if the space remaining in original buffer after the first network packet is not enough to contain at least another network packet descriptor, it is not cloned. The loop parsing this packets ended if remaining space == 0. But if the remaining space was > 0 but < packet descriptor size, another iteration of the loop was done, processing again the previous packet because cloning didn't happen. Moreover, the ownership of this packet had been passed to network stack in the previous iteration. This patch ensures that no extra iteration is done if the remaining size is not enough for one packet, and also avoid the first iteration for the same reason. Probably this doesn't happen in practice, but can happen theoretically. Signed-off-by: Íñigo Huguet <ihuguet@redhat.com> --- drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)