| Message ID | 20201030022839.438135-4-xie.he.0141@gmail.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | net: hdlc_fr: Improve fr_rx and add support for any Ethertype | expand |
| 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 |
On Thu, Oct 29, 2020 at 10:33 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. Can you elaborate a bit for readers not intimately familiar with the codebase? Is there something in the following code that has this implicit assumption on 2-byte address lengths?
On Fri, Oct 30, 2020 at 9:31 AM Willem de Bruijn <willemdebruijn.kernel@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. > > Can you elaborate a bit for readers not intimately familiar with the codebase? > > Is there something in the following code that has this implicit > assumption on 2-byte address lengths? Yes, the address length must be 2 bytes, otherwise the 3rd and 4th bytes would not be the control and protocol fields as we assumed in the code. The frame format is specified in RFC 2427 (https://tools.ietf.org/html/rfc2427). We can see the overall frame format on Page 3. If the address length is longer than 2 bytes, all the following fields will be shifted behind.
On Fri, Oct 30, 2020 at 3:21 PM Xie He <xie.he.0141@gmail.com> wrote: > > On Fri, Oct 30, 2020 at 9:31 AM Willem de Bruijn > <willemdebruijn.kernel@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. > > > > Can you elaborate a bit for readers not intimately familiar with the codebase? > > > > Is there something in the following code that has this implicit > > assumption on 2-byte address lengths? > > Yes, the address length must be 2 bytes, otherwise the 3rd and 4th > bytes would not be the control and protocol fields as we assumed in > the code. > > The frame format is specified in RFC 2427 > (https://tools.ietf.org/html/rfc2427). We can see the overall frame > format on Page 3. If the address length is longer than 2 bytes, all > the following fields will be shifted behind. Thanks for that context. If it's not captured in the code, it would be great to include in the commit message. From a quick scan, RFC 2427 does not appear to actually define the Q.922 address. For that I ended up reading ITU-T doc "Q.922 : ISDN data link layer specification for frame mode bearer services", section 3.2.
On Fri, Oct 30, 2020 at 2:20 PM Willem de Bruijn <willemdebruijn.kernel@gmail.com> wrote: > > Thanks for that context. If it's not captured in the code, it would be > great to include in the commit message. OK. I'll update the commit message. > From a quick scan, RFC 2427 does not appear to actually define the > Q.922 address. For that I ended up reading ITU-T doc "Q.922 : ISDN > data link layer specification for frame mode bearer services", section > 3.2. Yeah. Thanks for posting the name of the document. It's good to see ITU's documents are published in multiple languages because this feels more international :)
diff --git a/drivers/net/wan/hdlc_fr.c b/drivers/net/wan/hdlc_fr.c index ac65f5c435ef..3639c2bfb141 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);
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. 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(-)