| Message ID | 20200601071953.23252-2-wright.feng@cypress.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | brcmfmac: Stability change series | expand |
On 6/1/2020 12:19 AM, Wright Feng wrote: > From: Raveendran Somu <raveendran.somu@cypress.com> > > To trunkcate the addtional bytes, if extra bytes been received. typo: truncate. Missing "have been received". > Current code only have a warning and proceed without handling it. > But in one of the crash reported by DVT, these causes the > crash intermittently. So the processing is limit to the skb->len. > > Signed-off-by: Raveendran Somu <raveendran.somu@cypress.com> > Signed-off-by: Chi-hsien Lin <chi-hsien.lin@cypress.com> > Signed-off-by: Wright Feng <wright.feng@cypress.com> > --- > drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.c > index 09701262330d..531fe9be4025 100644 > --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.c > +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.c > @@ -1843,6 +1843,9 @@ void brcmf_fws_hdrpull(struct brcmf_if *ifp, s16 siglen, struct sk_buff *skb) > > WARN_ON(siglen > skb->len); > > + if (siglen > skb->len) > + siglen = skb->len; Does it make sense to keep the WARN_ON() one live above then? > + > if (!siglen) > return; > /* if flow control disabled, skip to packet data and leave */ >
Florian Fainelli 於 6/2/2020 12:34 PM 寫道: > > > On 6/1/2020 12:19 AM, Wright Feng wrote: >> From: Raveendran Somu <raveendran.somu@cypress.com> >> >> To trunkcate the addtional bytes, if extra bytes been received. > > typo: truncate. Missing "have been received". Thanks for catching this, Raveendran will correct it in V2. > >> Current code only have a warning and proceed without handling it. >> But in one of the crash reported by DVT, these causes the >> crash intermittently. So the processing is limit to the skb->len. >> >> Signed-off-by: Raveendran Somu <raveendran.somu@cypress.com> >> Signed-off-by: Chi-hsien Lin <chi-hsien.lin@cypress.com> >> Signed-off-by: Wright Feng <wright.feng@cypress.com> >> --- >> drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.c | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.c >> index 09701262330d..531fe9be4025 100644 >> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.c >> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.c >> @@ -1843,6 +1843,9 @@ void brcmf_fws_hdrpull(struct brcmf_if *ifp, s16 siglen, struct sk_buff *skb) >> >> WARN_ON(siglen > skb->len); >> >> + if (siglen > skb->len) >> + siglen = skb->len; > > Does it make sense to keep the WARN_ON() one live above then? Regarding the change, I believe it is better to have the WARN_ON as getting something more than expected is indicating the violation of protocol.So it may be better to leave the warning, instead of quietly correctly it, which can help in identifying the malicious operations. If the warning is not complying with the rules, we can remove it, if not we suggest to have it. >> + >> if (!siglen) >> return; >> /* if flow control disabled, skip to packet data and leave */ >> >
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.c index 09701262330d..531fe9be4025 100644 --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.c +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.c @@ -1843,6 +1843,9 @@ void brcmf_fws_hdrpull(struct brcmf_if *ifp, s16 siglen, struct sk_buff *skb) WARN_ON(siglen > skb->len); + if (siglen > skb->len) + siglen = skb->len; + if (!siglen) return; /* if flow control disabled, skip to packet data and leave */