| Message ID | a03166fcc8214644333c68674a781836e0f57576.1612697217.git.lorenzo@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | [wireless-drivers] mt76: dma: do not report truncated frames to mac80211 | expand |
Lorenzo Bianconi <lorenzo@kernel.org> writes: > If the fragment is discarded in mt76_add_fragment() since shared_info > frag array is full, discard truncated frames and do not forward them to > mac80211. > > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org> Should there be a Fixes line? I can add it. -- https://patchwork.kernel.org/project/linux-wireless/list/ https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
> Lorenzo Bianconi <lorenzo@kernel.org> writes: > > > If the fragment is discarded in mt76_add_fragment() since shared_info > > frag array is full, discard truncated frames and do not forward them to > > mac80211. > > > > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org> > > Should there be a Fixes line? I can add it. I am not sure it needs a Fixes tag. If so you can use: 93a1d4791c10 ("mt76: dma: fix a possible memory leak in mt76_add_fragment()") Regsrds, Lorenzo > > -- > https://patchwork.kernel.org/project/linux-wireless/list/ > > https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
Lorenzo Bianconi <lorenzo@kernel.org> writes: >> Lorenzo Bianconi <lorenzo@kernel.org> writes: >> >> > If the fragment is discarded in mt76_add_fragment() since shared_info >> > frag array is full, discard truncated frames and do not forward them to >> > mac80211. >> > >> > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org> >> >> Should there be a Fixes line? I can add it. > > I am not sure it needs a Fixes tag. I think the commit log should have some kind of description about the background of the issue, for example if this is a recent regression or has been there forever etc. -- https://patchwork.kernel.org/project/linux-wireless/list/ https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
> Lorenzo Bianconi <lorenzo@kernel.org> writes: > > >> Lorenzo Bianconi <lorenzo@kernel.org> writes: > >> > >> > If the fragment is discarded in mt76_add_fragment() since shared_info > >> > frag array is full, discard truncated frames and do not forward them to > >> > mac80211. > >> > > >> > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org> > >> > >> Should there be a Fixes line? I can add it. > > > > I am not sure it needs a Fixes tag. > > I think the commit log should have some kind of description about the > background of the issue, for example if this is a recent regression or > has been there forever etc. Agree. Can you please check the commit log below? Regards, Lorenzo " Commit 'b102f0c522cf6 ("mt76: fix array overflow on receiving too many fragments for a packet")' fixes a possible OOB access but it introduces a memory leak since the pending frame is not released to page_frag_cache if the frag array of skb_shared_info is full. Commit '93a1d4791c10 ("mt76: dma: fix a possible memory leak in mt76_add_fragment()")' fixes the issue but does not free the truncated skb that is forwarded to mac80211 layer. Fix the leftover issue discarding even truncated skbs. " > > -- > https://patchwork.kernel.org/project/linux-wireless/list/ > > https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
Lorenzo Bianconi <lorenzo@kernel.org> writes: >> Lorenzo Bianconi <lorenzo@kernel.org> writes: >> >> >> Lorenzo Bianconi <lorenzo@kernel.org> writes: >> >> >> >> > If the fragment is discarded in mt76_add_fragment() since shared_info >> >> > frag array is full, discard truncated frames and do not forward them to >> >> > mac80211. >> >> > >> >> > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org> >> >> >> >> Should there be a Fixes line? I can add it. >> > >> > I am not sure it needs a Fixes tag. >> >> I think the commit log should have some kind of description about the >> background of the issue, for example if this is a recent regression or >> has been there forever etc. > > Agree. Can you please check the commit log below? > > " > Commit 'b102f0c522cf6 ("mt76: fix array overflow on receiving too many > fragments for a packet")' fixes a possible OOB access but it introduces a > memory leak since the pending frame is not released to page_frag_cache if > the frag array of skb_shared_info is full. > Commit '93a1d4791c10 ("mt76: dma: fix a possible memory leak in > mt76_add_fragment()")' fixes the issue but does not free the truncated skb that > is forwarded to mac80211 layer. Fix the leftover issue discarding even truncated > skbs. > " Looks good, but I think the recommended style for commit ids is not to use ' chararacter. So I would change it to this: ---------------------------------------------------------------------- Commit b102f0c522cf6 ("mt76: fix array overflow on receiving too many fragments for a packet") fixes a possible OOB access but it introduces a memory leak since the pending frame is not released to page_frag_cache if the frag array of skb_shared_info is full. Commit 93a1d4791c10 ("mt76: dma: fix a possible memory leak in mt76_add_fragment()") fixes the issue but does not free the truncated skb that is forwarded to mac80211 layer. Fix the leftover issue discarding even truncated skbs. ---------------------------------------------------------------------- Should I add that to the commit log and queue the patch to be applied after the merge window opens? -- https://patchwork.kernel.org/project/linux-wireless/list/ https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
> Lorenzo Bianconi <lorenzo@kernel.org> writes: > > >> Lorenzo Bianconi <lorenzo@kernel.org> writes: > >> > >> >> Lorenzo Bianconi <lorenzo@kernel.org> writes: > >> >> > >> >> > If the fragment is discarded in mt76_add_fragment() since shared_info > >> >> > frag array is full, discard truncated frames and do not forward them to > >> >> > mac80211. > >> >> > > >> >> > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org> > >> >> > >> >> Should there be a Fixes line? I can add it. > >> > > >> > I am not sure it needs a Fixes tag. > >> > >> I think the commit log should have some kind of description about the > >> background of the issue, for example if this is a recent regression or > >> has been there forever etc. > > > > Agree. Can you please check the commit log below? > > > > " > > Commit 'b102f0c522cf6 ("mt76: fix array overflow on receiving too many > > fragments for a packet")' fixes a possible OOB access but it introduces a > > memory leak since the pending frame is not released to page_frag_cache if > > the frag array of skb_shared_info is full. > > Commit '93a1d4791c10 ("mt76: dma: fix a possible memory leak in > > mt76_add_fragment()")' fixes the issue but does not free the truncated skb that > > is forwarded to mac80211 layer. Fix the leftover issue discarding even truncated > > skbs. > > " > > Looks good, but I think the recommended style for commit ids is not to > use ' chararacter. So I would change it to this: > > ---------------------------------------------------------------------- > Commit b102f0c522cf6 ("mt76: fix array overflow on receiving too many > fragments for a packet") fixes a possible OOB access but it introduces a > memory leak since the pending frame is not released to page_frag_cache > if the frag array of skb_shared_info is full. Commit 93a1d4791c10 > ("mt76: dma: fix a possible memory leak in mt76_add_fragment()") fixes > the issue but does not free the truncated skb that is forwarded to > mac80211 layer. Fix the leftover issue discarding even truncated skbs. > ---------------------------------------------------------------------- > > Should I add that to the commit log and queue the patch to be applied > after the merge window opens? ack, fine to me, thx. Regards, Lorenzo > > -- > https://patchwork.kernel.org/project/linux-wireless/list/ > > https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
> > Lorenzo Bianconi <lorenzo@kernel.org> writes: > > >> Lorenzo Bianconi <lorenzo@kernel.org> writes: > >> > >> >> Lorenzo Bianconi <lorenzo@kernel.org> writes: > >> >> > >> >> > If the fragment is discarded in mt76_add_fragment() since shared_info > >> >> > frag array is full, discard truncated frames and do not forward them to > >> >> > mac80211. > >> >> > > >> >> > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org> > >> >> > >> >> Should there be a Fixes line? I can add it. > >> > > >> > I am not sure it needs a Fixes tag. > >> > >> I think the commit log should have some kind of description about the > >> background of the issue, for example if this is a recent regression or > >> has been there forever etc. > > > > Agree. Can you please check the commit log below? > > > > " > > Commit 'b102f0c522cf6 ("mt76: fix array overflow on receiving too many > > fragments for a packet")' fixes a possible OOB access but it introduces a > > memory leak since the pending frame is not released to page_frag_cache if > > the frag array of skb_shared_info is full. > > Commit '93a1d4791c10 ("mt76: dma: fix a possible memory leak in > > mt76_add_fragment()")' fixes the issue but does not free the truncated skb that > > is forwarded to mac80211 layer. Fix the leftover issue discarding even truncated > > skbs. > > " > > Looks good, but I think the recommended style for commit ids is not to > use ' chararacter. So I would change it to this: > > ---------------------------------------------------------------------- > Commit b102f0c522cf6 ("mt76: fix array overflow on receiving too many > fragments for a packet") fixes a possible OOB access but it introduces a > memory leak since the pending frame is not released to page_frag_cache > if the frag array of skb_shared_info is full. Commit 93a1d4791c10 > ("mt76: dma: fix a possible memory leak in mt76_add_fragment()") fixes > the issue but does not free the truncated skb that is forwarded to > mac80211 layer. Fix the leftover issue discarding even truncated skbs. > ---------------------------------------------------------------------- > > Should I add that to the commit log and queue the patch to be applied > after the merge window opens? > > -- > https://patchwork.kernel.org/project/linux-wireless/list/ > > https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches > Hi Kalle, any news about this patch? Regards, Lorenzo
Lorenzo Bianconi <lorenzo.bianconi@redhat.com> writes: >> >> Lorenzo Bianconi <lorenzo@kernel.org> writes: >> >> >> Lorenzo Bianconi <lorenzo@kernel.org> writes: >> >> >> >> >> Lorenzo Bianconi <lorenzo@kernel.org> writes: >> >> >> >> >> >> > If the fragment is discarded in mt76_add_fragment() since shared_info >> >> >> > frag array is full, discard truncated frames and do not forward them to >> >> >> > mac80211. >> >> >> > >> >> >> > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org> >> >> >> >> >> >> Should there be a Fixes line? I can add it. >> >> > >> >> > I am not sure it needs a Fixes tag. >> >> >> >> I think the commit log should have some kind of description about the >> >> background of the issue, for example if this is a recent regression or >> >> has been there forever etc. >> > >> > Agree. Can you please check the commit log below? >> > >> > " >> > Commit 'b102f0c522cf6 ("mt76: fix array overflow on receiving too many >> > fragments for a packet")' fixes a possible OOB access but it introduces a >> > memory leak since the pending frame is not released to page_frag_cache if >> > the frag array of skb_shared_info is full. >> > Commit '93a1d4791c10 ("mt76: dma: fix a possible memory leak in >> > mt76_add_fragment()")' fixes the issue but does not free the truncated skb that >> > is forwarded to mac80211 layer. Fix the leftover issue discarding even truncated >> > skbs. >> > " >> >> Looks good, but I think the recommended style for commit ids is not to >> use ' chararacter. So I would change it to this: >> >> ---------------------------------------------------------------------- >> Commit b102f0c522cf6 ("mt76: fix array overflow on receiving too many >> fragments for a packet") fixes a possible OOB access but it introduces a >> memory leak since the pending frame is not released to page_frag_cache >> if the frag array of skb_shared_info is full. Commit 93a1d4791c10 >> ("mt76: dma: fix a possible memory leak in mt76_add_fragment()") fixes >> the issue but does not free the truncated skb that is forwarded to >> mac80211 layer. Fix the leftover issue discarding even truncated skbs. >> ---------------------------------------------------------------------- >> >> Should I add that to the commit log and queue the patch to be applied >> after the merge window opens? > > any news about this patch? It was not assigned to me on patchwork so it was not on my radar. I now assigned it to me. -- https://patchwork.kernel.org/project/linux-wireless/list/ https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
Lorenzo Bianconi <lorenzo@kernel.org> wrote: > Commit b102f0c522cf6 ("mt76: fix array overflow on receiving too many > fragments for a packet") fixes a possible OOB access but it introduces a > memory leak since the pending frame is not released to page_frag_cache > if the frag array of skb_shared_info is full. Commit 93a1d4791c10 > ("mt76: dma: fix a possible memory leak in mt76_add_fragment()") fixes > the issue but does not free the truncated skb that is forwarded to > mac80211 layer. Fix the leftover issue discarding even truncated skbs. > > Fixes: 93a1d4791c10 ("mt76: dma: fix a possible memory leak in mt76_add_fragment()") > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org> Patch applied to wireless-drivers.git, thanks. d0bd52c591a1 mt76: dma: do not report truncated frames to mac80211 -- https://patchwork.kernel.org/project/linux-wireless/patch/a03166fcc8214644333c68674a781836e0f57576.1612697217.git.lorenzo@kernel.org/ https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
diff --git a/drivers/net/wireless/mediatek/mt76/dma.c b/drivers/net/wireless/mediatek/mt76/dma.c index e81dfaf99bcb..9bf13994c036 100644 --- a/drivers/net/wireless/mediatek/mt76/dma.c +++ b/drivers/net/wireless/mediatek/mt76/dma.c @@ -511,13 +511,13 @@ mt76_add_fragment(struct mt76_dev *dev, struct mt76_queue *q, void *data, { struct sk_buff *skb = q->rx_head; struct skb_shared_info *shinfo = skb_shinfo(skb); + int nr_frags = shinfo->nr_frags; - if (shinfo->nr_frags < ARRAY_SIZE(shinfo->frags)) { + if (nr_frags < ARRAY_SIZE(shinfo->frags)) { struct page *page = virt_to_head_page(data); int offset = data - page_address(page) + q->buf_offset; - skb_add_rx_frag(skb, shinfo->nr_frags, page, offset, len, - q->buf_size); + skb_add_rx_frag(skb, nr_frags, page, offset, len, q->buf_size); } else { skb_free_frag(data); } @@ -526,7 +526,10 @@ mt76_add_fragment(struct mt76_dev *dev, struct mt76_queue *q, void *data, return; q->rx_head = NULL; - dev->drv->rx_skb(dev, q - dev->q_rx, skb); + if (nr_frags < ARRAY_SIZE(shinfo->frags)) + dev->drv->rx_skb(dev, q - dev->q_rx, skb); + else + dev_kfree_skb(skb); } static int
If the fragment is discarded in mt76_add_fragment() since shared_info frag array is full, discard truncated frames and do not forward them to mac80211. Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org> --- drivers/net/wireless/mediatek/mt76/dma.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-)