| Message ID | e5ea0147b3314ad9db5140c7b307472efbd114bd.1677888566.git.lucien.xin@gmail.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Pablo Neira |
| Headers | show |
| Series | netfilter: handle ipv6 jumbo packets properly for bridge ovs and tc | expand |
On Fri, Mar 03, 2023 at 07:12:38PM -0500, Xin Long wrote: > In the while loop of br_nf_check_hbh_len(), similar to ip6_parse_tlv(), > before accessing 'nh[off + 1]', it should add a check 'len < 2'; and > before parsing IPV6_TLV_JUMBO, it should add a check 'optlen > len', > in case of overflows. > > Signed-off-by: Xin Long <lucien.xin@gmail.com> Reviewed-by: Simon Horman <simon.horman@corigine.com> > --- > net/bridge/br_netfilter_ipv6.c | 47 ++++++++++++++++------------------ > 1 file changed, 22 insertions(+), 25 deletions(-) > > diff --git a/net/bridge/br_netfilter_ipv6.c b/net/bridge/br_netfilter_ipv6.c > index 5cd3e4c35123..50f564c33551 100644 > --- a/net/bridge/br_netfilter_ipv6.c > +++ b/net/bridge/br_netfilter_ipv6.c ... > - if (len == 0) > - return 0; > -bad: > - return -1; > + if (len) > + return -1; > + > + return 0; nit: if you have to spin a v2, you may want to consider return len ? -1 : 0;
On 04/03/2023 02:12, Xin Long wrote: > In the while loop of br_nf_check_hbh_len(), similar to ip6_parse_tlv(), > before accessing 'nh[off + 1]', it should add a check 'len < 2'; and > before parsing IPV6_TLV_JUMBO, it should add a check 'optlen > len', > in case of overflows. > > Signed-off-by: Xin Long <lucien.xin@gmail.com> > --- > net/bridge/br_netfilter_ipv6.c | 47 ++++++++++++++++------------------ > 1 file changed, 22 insertions(+), 25 deletions(-) Acked-by: Nikolay Aleksandrov <razor@blackwall.org>
Xin Long <lucien.xin@gmail.com> writes: > In the while loop of br_nf_check_hbh_len(), similar to ip6_parse_tlv(), > before accessing 'nh[off + 1]', it should add a check 'len < 2'; and > before parsing IPV6_TLV_JUMBO, it should add a check 'optlen > len', > in case of overflows. > > Signed-off-by: Xin Long <lucien.xin@gmail.com> > --- Reviewed-by: Aaron Conole <aconole@redhat.com> > net/bridge/br_netfilter_ipv6.c | 47 ++++++++++++++++------------------ > 1 file changed, 22 insertions(+), 25 deletions(-) > > diff --git a/net/bridge/br_netfilter_ipv6.c b/net/bridge/br_netfilter_ipv6.c > index 5cd3e4c35123..50f564c33551 100644 > --- a/net/bridge/br_netfilter_ipv6.c > +++ b/net/bridge/br_netfilter_ipv6.c > @@ -50,54 +50,51 @@ static int br_nf_check_hbh_len(struct sk_buff *skb) > u32 pkt_len; > > if (!pskb_may_pull(skb, off + 8)) > - goto bad; > + return -1; > nh = (u8 *)(ipv6_hdr(skb) + 1); > len = (nh[1] + 1) << 3; > > if (!pskb_may_pull(skb, off + len)) > - goto bad; > + return -1; > nh = skb_network_header(skb); > > off += 2; > len -= 2; > - > while (len > 0) { > - int optlen = nh[off + 1] + 2; > - > - switch (nh[off]) { > - case IPV6_TLV_PAD1: > - optlen = 1; > - break; > + int optlen; > > - case IPV6_TLV_PADN: > - break; > + if (nh[off] == IPV6_TLV_PAD1) { > + off++; > + len--; > + continue; > + } > + if (len < 2) > + return -1; > + optlen = nh[off + 1] + 2; > + if (optlen > len) > + return -1; > > - case IPV6_TLV_JUMBO: > + if (nh[off] == IPV6_TLV_JUMBO) { > if (nh[off + 1] != 4 || (off & 3) != 2) > - goto bad; > + return -1; > pkt_len = ntohl(*(__be32 *)(nh + off + 2)); > if (pkt_len <= IPV6_MAXPLEN || > ipv6_hdr(skb)->payload_len) > - goto bad; > + return -1; > if (pkt_len > skb->len - sizeof(struct ipv6hdr)) > - goto bad; > + return -1; > if (pskb_trim_rcsum(skb, > pkt_len + sizeof(struct ipv6hdr))) > - goto bad; > + return -1; > nh = skb_network_header(skb); > - break; > - default: > - if (optlen > len) > - goto bad; > - break; > } > off += optlen; > len -= optlen; > } > - if (len == 0) > - return 0; > -bad: > - return -1; > + if (len) > + return -1; > + > + return 0; > } > > int br_validate_ipv6(struct net *net, struct sk_buff *skb)
diff --git a/net/bridge/br_netfilter_ipv6.c b/net/bridge/br_netfilter_ipv6.c index 5cd3e4c35123..50f564c33551 100644 --- a/net/bridge/br_netfilter_ipv6.c +++ b/net/bridge/br_netfilter_ipv6.c @@ -50,54 +50,51 @@ static int br_nf_check_hbh_len(struct sk_buff *skb) u32 pkt_len; if (!pskb_may_pull(skb, off + 8)) - goto bad; + return -1; nh = (u8 *)(ipv6_hdr(skb) + 1); len = (nh[1] + 1) << 3; if (!pskb_may_pull(skb, off + len)) - goto bad; + return -1; nh = skb_network_header(skb); off += 2; len -= 2; - while (len > 0) { - int optlen = nh[off + 1] + 2; - - switch (nh[off]) { - case IPV6_TLV_PAD1: - optlen = 1; - break; + int optlen; - case IPV6_TLV_PADN: - break; + if (nh[off] == IPV6_TLV_PAD1) { + off++; + len--; + continue; + } + if (len < 2) + return -1; + optlen = nh[off + 1] + 2; + if (optlen > len) + return -1; - case IPV6_TLV_JUMBO: + if (nh[off] == IPV6_TLV_JUMBO) { if (nh[off + 1] != 4 || (off & 3) != 2) - goto bad; + return -1; pkt_len = ntohl(*(__be32 *)(nh + off + 2)); if (pkt_len <= IPV6_MAXPLEN || ipv6_hdr(skb)->payload_len) - goto bad; + return -1; if (pkt_len > skb->len - sizeof(struct ipv6hdr)) - goto bad; + return -1; if (pskb_trim_rcsum(skb, pkt_len + sizeof(struct ipv6hdr))) - goto bad; + return -1; nh = skb_network_header(skb); - break; - default: - if (optlen > len) - goto bad; - break; } off += optlen; len -= optlen; } - if (len == 0) - return 0; -bad: - return -1; + if (len) + return -1; + + return 0; } int br_validate_ipv6(struct net *net, struct sk_buff *skb)
In the while loop of br_nf_check_hbh_len(), similar to ip6_parse_tlv(), before accessing 'nh[off + 1]', it should add a check 'len < 2'; and before parsing IPV6_TLV_JUMBO, it should add a check 'optlen > len', in case of overflows. Signed-off-by: Xin Long <lucien.xin@gmail.com> --- net/bridge/br_netfilter_ipv6.c | 47 ++++++++++++++++------------------ 1 file changed, 22 insertions(+), 25 deletions(-)