diff mbox series

[net-next,v6,4/5] net: hdlc_fr: Improve the initial checks when we receive an skb

Message ID 20201031004918.463475-5-xie.he.0141@gmail.com
State Superseded
Delegated to: David Miller
Headers show
Series net: hdlc_fr: Improve fr_rx and add support for any Ethertype | expand

Checks

Context Check Description
jkicinski/cover_letter success Link
jkicinski/fixes_present success Link
jkicinski/patch_count success Link
jkicinski/tree_selection success Clearly marked for net-next
jkicinski/subject_prefix success Link
jkicinski/source_inline success Was 0 now: 0
jkicinski/verify_signedoff success Link
jkicinski/module_param success Was 0 now: 0
jkicinski/build_32bit success Errors and warnings before: 0 this patch: 0
jkicinski/kdoc success Errors and warnings before: 0 this patch: 0
jkicinski/verify_fixes success Link
jkicinski/checkpatch success total: 0 errors, 0 warnings, 0 checks, 8 lines checked
jkicinski/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
jkicinski/header_inline success Link
jkicinski/stable success Stable not CCed

Commit Message

Xie He Oct. 31, 2020, 12:49 a.m. UTC
1.
Change the skb->len check from "<= 4" to "< 4".
At first we only need to ensure a 4-byte header is present. We indeed
normally need the 5th byte, too, but it'd be more logical and cleaner
to check its existence when we actually need it.

2.
Add an fh->ea2 check to the initial checks in fr_rx. fh->ea2 == 1 means
the second address byte is the final address byte. We only support the
case where the address length is 2 bytes. If the address length is not
2 bytes, the control field and the protocol field would not be the 3rd
and 4th byte as we assume. (Say it is 3 bytes, then the control field
and the protocol field would be the 4th and 5th byte instead.)

Cc: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
Cc: Krzysztof Halasa <khc@pm.waw.pl>
Signed-off-by: Xie He <xie.he.0141@gmail.com>
---
 drivers/net/wan/hdlc_fr.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Willem de Bruijn Oct. 31, 2020, 2:25 p.m. UTC | #1
On Fri, Oct 30, 2020 at 8:49 PM Xie He <xie.he.0141@gmail.com> wrote:
>
> 1.
> Change the skb->len check from "<= 4" to "< 4".
> At first we only need to ensure a 4-byte header is present. We indeed
> normally need the 5th byte, too, but it'd be more logical and cleaner
> to check its existence when we actually need it.
>
> 2.
> Add an fh->ea2 check to the initial checks in fr_rx. fh->ea2 == 1 means
> the second address byte is the final address byte. We only support the
> case where the address length is 2 bytes. If the address length is not
> 2 bytes, the control field and the protocol field would not be the 3rd
> and 4th byte as we assume. (Say it is 3 bytes, then the control field
> and the protocol field would be the 4th and 5th byte instead.)
>
> Cc: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
> Cc: Krzysztof Halasa <khc@pm.waw.pl>
> Signed-off-by: Xie He <xie.he.0141@gmail.com>

Acked-by: Willem de Bruijn <willemb@google.com>
Xie He Oct. 31, 2020, 5:05 p.m. UTC | #2
On Fri, Oct 30, 2020 at 5:49 PM Xie He <xie.he.0141@gmail.com> wrote:
>
> Add an fh->ea2 check to the initial checks in fr_rx. fh->ea2 == 1 means
> the second address byte is the final address byte. We only support the
> case where the address length is 2 bytes. If the address length is not
> 2 bytes, the control field and the protocol field would not be the 3rd
> and 4th byte as we assume. (Say it is 3 bytes, then the control field
> and the protocol field would be the 4th and 5th byte instead.)

No, I don't think adding this explanation (about why address lengths
other than 2 are not supported) is necessary. The code of this driver
completely doesn't support address lengths other than 2 for many
reasons, not only the reason above. The code for parsing and
generating the address field (q922_to_dlci and dlci_to_q922) supports
only 2-byte address fields. Also, the structure of the sending code of
this driver can only generate headers with a 2-byte address field.
Frame Relay networks usually have a fixed address length, that means a
certain network usually has only 1 fixed address length. If this
driver sends frames with 2-byte address fields, it shouldn't expect it
would receive frames with 3-byte or 4-byte address fields. There are
too many reasons, and I think just saying we only support 2-byte
address fields is completely enough. Explaining the reason as above
seems weird and inadequate to me, and also unnecessary to me.

Therefore, I will resend this patch series with this explanation
(about why address lengths other than 2 are not supported) removed.
diff mbox series

Patch

diff --git a/drivers/net/wan/hdlc_fr.c b/drivers/net/wan/hdlc_fr.c
index eb83116aa9df..98444f1d8cc3 100644
--- a/drivers/net/wan/hdlc_fr.c
+++ b/drivers/net/wan/hdlc_fr.c
@@ -882,7 +882,7 @@  static int fr_rx(struct sk_buff *skb)
 	struct pvc_device *pvc;
 	struct net_device *dev;
 
-	if (skb->len <= 4 || fh->ea1 || data[2] != FR_UI)
+	if (skb->len < 4 || fh->ea1 || !fh->ea2 || data[2] != FR_UI)
 		goto rx_error;
 
 	dlci = q922_to_dlci(skb->data);