diff mbox series

[net-next,v1] net: ipv6: hop tlv ext hdr parsing

Message ID 9b82bb29-9316-4dfd-8c56-f8a294713c16@gmail.com (mailing list archive)
State Changes Requested
Delegated to: Netdev Maintainers
Headers show
Series [net-next,v1] net: ipv6: hop tlv ext hdr parsing | expand

Checks

Context Check Description
netdev/series_format success Single patches do not need cover letters
netdev/tree_selection success Clearly marked for net-next
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 2809 this patch: 2809
netdev/build_tools success Errors and warnings before: 0 this patch: 0
netdev/cc_maintainers success CCed 5 of 5 maintainers
netdev/build_clang success Errors and warnings before: 997 this patch: 997
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 3022 this patch: 3022
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 94 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
netdev/contest success net-next-2024-02-21--12-00 (tests: 1454)

Commit Message

Richard Gobert Feb. 21, 2024, 10:03 a.m. UTC 
This patch introduces 'hop_tlv_hdr' and 'hop_calipso_hdr' structs, in
order to access fields in a readable way in "ip6_parse_tlv".

Signed-off-by: Richard Gobert <richardbgobert@gmail.com>
---
 include/net/ipv6.h | 16 ++++++++++++++++
 net/ipv6/exthdrs.c | 30 +++++++++++++++++-------------
 2 files changed, 33 insertions(+), 13 deletions(-)

Comments

Denis Kirjanov Feb. 21, 2024, 10:20 a.m. UTC | #1 
On 2/21/24 13:03, Richard Gobert wrote:
> This patch introduces 'hop_tlv_hdr' and 'hop_calipso_hdr' structs, in
> order to access fields in a readable way in "ip6_parse_tlv".
> 
> Signed-off-by: Richard Gobert <richardbgobert@gmail.com>
> ---
>  include/net/ipv6.h | 16 ++++++++++++++++
>  net/ipv6/exthdrs.c | 30 +++++++++++++++++-------------
>  2 files changed, 33 insertions(+), 13 deletions(-)
> 
> diff --git a/include/net/ipv6.h b/include/net/ipv6.h
> index cf25ea21d770..61677946ed46 100644
> --- a/include/net/ipv6.h
> +++ b/include/net/ipv6.h
> @@ -151,6 +151,22 @@ struct frag_hdr {
>  	__be32	identification;
>  };
>  
> +struct hop_tlv_hdr {
> +	u8	tlv_type;
> +	u8	tlv_len;
> +};
> +
> +/* CALIPSO RFC 5570 */
> +struct hop_calipso_hdr {
> +	u8	tlv_type;
> +	u8	tlv_len;
> +	u32	domain_interpretation;
> +	u8	cmpt_len;
> +	u8	sens_lvl;
> +	u16	checksum;
> +	u64	cmpt_bitmap;
> +} __packed;
> +
>  /*
>   * Jumbo payload option, as described in RFC 2675 2.
>   */
> diff --git a/net/ipv6/exthdrs.c b/net/ipv6/exthdrs.c
> index 4952ae792450..5db624299da4 100644
> --- a/net/ipv6/exthdrs.c
> +++ b/net/ipv6/exthdrs.c
> @@ -114,7 +114,7 @@ static bool ip6_parse_tlv(bool hopbyhop,
>  			  struct sk_buff *skb,
>  			  int max_count)
>  {
> -	int len = (skb_transport_header(skb)[1] + 1) << 3;
> +	int len = ipv6_optlen((struct ipv6_opt_hdr *)skb_transport_header(skb));
>  	const unsigned char *nh = skb_network_header(skb);
>  	int off = skb_network_header_len(skb);
>  	bool disallow_unknowns = false;
> @@ -890,15 +890,16 @@ static inline struct net *ipv6_skb_net(struct sk_buff *skb)
>  
>  static bool ipv6_hop_ra(struct sk_buff *skb, int optoff)
>  {
> -	const unsigned char *nh = skb_network_header(skb);
> +	struct hop_tlv_hdr *tlv_hdr =
You've forgotten to contstify your struct here and below

> +		(struct hop_tlv_hdr *)(skb_network_header(skb) + optoff);
>  
> -	if (nh[optoff + 1] == 2) {
> +	if (tlv_hdr->tlv_len == 2) {
>  		IP6CB(skb)->flags |= IP6SKB_ROUTERALERT;
> -		memcpy(&IP6CB(skb)->ra, nh + optoff + 2, sizeof(IP6CB(skb)->ra));
> +		memcpy(&IP6CB(skb)->ra, tlv_hdr + 1, sizeof(IP6CB(skb)->ra));
>  		return true;
>  	}
>  	net_dbg_ratelimited("ipv6_hop_ra: wrong RA length %d\n",
> -			    nh[optoff + 1]);
> +			    tlv_hdr->tlv_len);
>  	kfree_skb_reason(skb, SKB_DROP_REASON_IP_INHDR);
>  	return false;
>  }
> @@ -961,18 +962,20 @@ static bool ipv6_hop_ioam(struct sk_buff *skb, int optoff)
>  
>  static bool ipv6_hop_jumbo(struct sk_buff *skb, int optoff)
>  {
> -	const unsigned char *nh = skb_network_header(skb);
> +	int tlv_off = offsetof(struct hop_jumbo_hdr, tlv_type);
> +	struct hop_jumbo_hdr *jumbo_hdr = (struct hop_jumbo_hdr *)

> +		(skb_network_header(skb) + optoff - tlv_off);
>  	SKB_DR(reason);
>  	u32 pkt_len;
>  
> -	if (nh[optoff + 1] != 4 || (optoff & 3) != 2) {
> +	if (jumbo_hdr->tlv_len != 4 || (optoff & 3) != 2) {
>  		net_dbg_ratelimited("ipv6_hop_jumbo: wrong jumbo opt length/alignment %d\n",
> -				    nh[optoff+1]);
> +				    jumbo_hdr->tlv_len);
>  		SKB_DR_SET(reason, IP_INHDR);
>  		goto drop;
>  	}
>  
> -	pkt_len = ntohl(*(__be32 *)(nh + optoff + 2));
> +	pkt_len = ntohl(jumbo_hdr->jumbo_payload_len);
>  	if (pkt_len <= IPV6_MAXPLEN) {
>  		icmpv6_param_prob_reason(skb, ICMPV6_HDR_FIELD, optoff + 2,
>  					 SKB_DROP_REASON_IP_INHDR);
> @@ -1004,15 +1007,16 @@ static bool ipv6_hop_jumbo(struct sk_buff *skb, int optoff)
>  
>  static bool ipv6_hop_calipso(struct sk_buff *skb, int optoff)
>  {
> -	const unsigned char *nh = skb_network_header(skb);
> +	struct hop_calipso_hdr *calipso_hdr =
> +		(struct hop_calipso_hdr *)(skb_network_header(skb) + optoff);
>  
> -	if (nh[optoff + 1] < 8)
> +	if (calipso_hdr->tlv_len < 8)
>  		goto drop;
>  
> -	if (nh[optoff + 6] * 4 + 8 > nh[optoff + 1])
> +	if (calipso_hdr->cmpt_len * 4 + 8 > calipso_hdr->tlv_len)
>  		goto drop;
>  
> -	if (!calipso_validate(skb, nh + optoff))
> +	if (!calipso_validate(skb, (const unsigned char *)calipso_hdr))
>  		goto drop;
>  
>  	return true;
David Ahern Feb. 22, 2024, 5:35 p.m. UTC | #2 
On 2/21/24 3:03 AM, Richard Gobert wrote:
> This patch introduces 'hop_tlv_hdr' and 'hop_calipso_hdr' structs, in
> order to access fields in a readable way in "ip6_parse_tlv".
> 
> Signed-off-by: Richard Gobert <richardbgobert@gmail.com>
> ---
>  include/net/ipv6.h | 16 ++++++++++++++++
>  net/ipv6/exthdrs.c | 30 +++++++++++++++++-------------
>  2 files changed, 33 insertions(+), 13 deletions(-)
> 
> diff --git a/include/net/ipv6.h b/include/net/ipv6.h
> index cf25ea21d770..61677946ed46 100644
> --- a/include/net/ipv6.h
> +++ b/include/net/ipv6.h
> @@ -151,6 +151,22 @@ struct frag_hdr {
>  	__be32	identification;
>  };
>  
> +struct hop_tlv_hdr {
> +	u8	tlv_type;
> +	u8	tlv_len;
> +};
> +
> +/* CALIPSO RFC 5570 */
> +struct hop_calipso_hdr {
> +	u8	tlv_type;
> +	u8	tlv_len;
> +	u32	domain_interpretation;
> +	u8	cmpt_len;
> +	u8	sens_lvl;
> +	u16	checksum;
> +	u64	cmpt_bitmap;
> +} __packed;
> +
>  /*
>   * Jumbo payload option, as described in RFC 2675 2.
>   */
> diff --git a/net/ipv6/exthdrs.c b/net/ipv6/exthdrs.c
> index 4952ae792450..5db624299da4 100644
> --- a/net/ipv6/exthdrs.c
> +++ b/net/ipv6/exthdrs.c
> @@ -114,7 +114,7 @@ static bool ip6_parse_tlv(bool hopbyhop,
>  			  struct sk_buff *skb,
>  			  int max_count)
>  {
> -	int len = (skb_transport_header(skb)[1] + 1) << 3;
> +	int len = ipv6_optlen((struct ipv6_opt_hdr *)skb_transport_header(skb));
>  	const unsigned char *nh = skb_network_header(skb);
>  	int off = skb_network_header_len(skb);
>  	bool disallow_unknowns = false;
> @@ -890,15 +890,16 @@ static inline struct net *ipv6_skb_net(struct sk_buff *skb)
>  
>  static bool ipv6_hop_ra(struct sk_buff *skb, int optoff)
>  {
> -	const unsigned char *nh = skb_network_header(skb);
> +	struct hop_tlv_hdr *tlv_hdr =
> +		(struct hop_tlv_hdr *)(skb_network_header(skb) + optoff);
>  
> -	if (nh[optoff + 1] == 2) {
> +	if (tlv_hdr->tlv_len == 2) {
>  		IP6CB(skb)->flags |= IP6SKB_ROUTERALERT;
> -		memcpy(&IP6CB(skb)->ra, nh + optoff + 2, sizeof(IP6CB(skb)->ra));
> +		memcpy(&IP6CB(skb)->ra, tlv_hdr + 1, sizeof(IP6CB(skb)->ra));
>  		return true;
>  	}
>  	net_dbg_ratelimited("ipv6_hop_ra: wrong RA length %d\n",
> -			    nh[optoff + 1]);
> +			    tlv_hdr->tlv_len);
>  	kfree_skb_reason(skb, SKB_DROP_REASON_IP_INHDR);
>  	return false;
>  }
> @@ -961,18 +962,20 @@ static bool ipv6_hop_ioam(struct sk_buff *skb, int optoff)
>  
>  static bool ipv6_hop_jumbo(struct sk_buff *skb, int optoff)
>  {
> -	const unsigned char *nh = skb_network_header(skb);
> +	int tlv_off = offsetof(struct hop_jumbo_hdr, tlv_type);
> +	struct hop_jumbo_hdr *jumbo_hdr = (struct hop_jumbo_hdr *)
> +		(skb_network_header(skb) + optoff - tlv_off);
>  	SKB_DR(reason);
>  	u32 pkt_len;
>  
> -	if (nh[optoff + 1] != 4 || (optoff & 3) != 2) {
> +	if (jumbo_hdr->tlv_len != 4 || (optoff & 3) != 2) {
>  		net_dbg_ratelimited("ipv6_hop_jumbo: wrong jumbo opt length/alignment %d\n",
> -				    nh[optoff+1]);
> +				    jumbo_hdr->tlv_len);
>  		SKB_DR_SET(reason, IP_INHDR);
>  		goto drop;
>  	}
>  
> -	pkt_len = ntohl(*(__be32 *)(nh + optoff + 2));
> +	pkt_len = ntohl(jumbo_hdr->jumbo_payload_len);
>  	if (pkt_len <= IPV6_MAXPLEN) {
>  		icmpv6_param_prob_reason(skb, ICMPV6_HDR_FIELD, optoff + 2,
>  					 SKB_DROP_REASON_IP_INHDR);
> @@ -1004,15 +1007,16 @@ static bool ipv6_hop_jumbo(struct sk_buff *skb, int optoff)
>  
>  static bool ipv6_hop_calipso(struct sk_buff *skb, int optoff)
>  {
> -	const unsigned char *nh = skb_network_header(skb);
> +	struct hop_calipso_hdr *calipso_hdr =
> +		(struct hop_calipso_hdr *)(skb_network_header(skb) + optoff);
>  
> -	if (nh[optoff + 1] < 8)
> +	if (calipso_hdr->tlv_len < 8)
>  		goto drop;
>  
> -	if (nh[optoff + 6] * 4 + 8 > nh[optoff + 1])
> +	if (calipso_hdr->cmpt_len * 4 + 8 > calipso_hdr->tlv_len)
>  		goto drop;
>  
> -	if (!calipso_validate(skb, nh + optoff))
> +	if (!calipso_validate(skb, (const unsigned char *)calipso_hdr))
>  		goto drop;
>  
>  	return true;

In addition to restoring the const that Denis commented, this really
wants to be multiple patches in a set. Also, it would be best to have
selftests as well that exercise the code paths.
Richard Gobert Feb. 26, 2024, 11:49 a.m. UTC | #3 
David Ahern wrote:

> In addition to restoring the const that Denis commented, this really
> wants to be multiple patches in a set. Also, it would be best to have
> selftests as well that exercise the code paths.
> 

Thanks for the comment.
I'll think about relevant selftests, and submit a v2 with the changes.
diff mbox series

Patch

diff --git a/include/net/ipv6.h b/include/net/ipv6.h
index cf25ea21d770..61677946ed46 100644
--- a/include/net/ipv6.h
+++ b/include/net/ipv6.h
@@ -151,6 +151,22 @@  struct frag_hdr {
 	__be32	identification;
 };
 
+struct hop_tlv_hdr {
+	u8	tlv_type;
+	u8	tlv_len;
+};
+
+/* CALIPSO RFC 5570 */
+struct hop_calipso_hdr {
+	u8	tlv_type;
+	u8	tlv_len;
+	u32	domain_interpretation;
+	u8	cmpt_len;
+	u8	sens_lvl;
+	u16	checksum;
+	u64	cmpt_bitmap;
+} __packed;
+
 /*
  * Jumbo payload option, as described in RFC 2675 2.
  */
diff --git a/net/ipv6/exthdrs.c b/net/ipv6/exthdrs.c
index 4952ae792450..5db624299da4 100644
--- a/net/ipv6/exthdrs.c
+++ b/net/ipv6/exthdrs.c
@@ -114,7 +114,7 @@  static bool ip6_parse_tlv(bool hopbyhop,
 			  struct sk_buff *skb,
 			  int max_count)
 {
-	int len = (skb_transport_header(skb)[1] + 1) << 3;
+	int len = ipv6_optlen((struct ipv6_opt_hdr *)skb_transport_header(skb));
 	const unsigned char *nh = skb_network_header(skb);
 	int off = skb_network_header_len(skb);
 	bool disallow_unknowns = false;
@@ -890,15 +890,16 @@  static inline struct net *ipv6_skb_net(struct sk_buff *skb)
 
 static bool ipv6_hop_ra(struct sk_buff *skb, int optoff)
 {
-	const unsigned char *nh = skb_network_header(skb);
+	struct hop_tlv_hdr *tlv_hdr =
+		(struct hop_tlv_hdr *)(skb_network_header(skb) + optoff);
 
-	if (nh[optoff + 1] == 2) {
+	if (tlv_hdr->tlv_len == 2) {
 		IP6CB(skb)->flags |= IP6SKB_ROUTERALERT;
-		memcpy(&IP6CB(skb)->ra, nh + optoff + 2, sizeof(IP6CB(skb)->ra));
+		memcpy(&IP6CB(skb)->ra, tlv_hdr + 1, sizeof(IP6CB(skb)->ra));
 		return true;
 	}
 	net_dbg_ratelimited("ipv6_hop_ra: wrong RA length %d\n",
-			    nh[optoff + 1]);
+			    tlv_hdr->tlv_len);
 	kfree_skb_reason(skb, SKB_DROP_REASON_IP_INHDR);
 	return false;
 }
@@ -961,18 +962,20 @@  static bool ipv6_hop_ioam(struct sk_buff *skb, int optoff)
 
 static bool ipv6_hop_jumbo(struct sk_buff *skb, int optoff)
 {
-	const unsigned char *nh = skb_network_header(skb);
+	int tlv_off = offsetof(struct hop_jumbo_hdr, tlv_type);
+	struct hop_jumbo_hdr *jumbo_hdr = (struct hop_jumbo_hdr *)
+		(skb_network_header(skb) + optoff - tlv_off);
 	SKB_DR(reason);
 	u32 pkt_len;
 
-	if (nh[optoff + 1] != 4 || (optoff & 3) != 2) {
+	if (jumbo_hdr->tlv_len != 4 || (optoff & 3) != 2) {
 		net_dbg_ratelimited("ipv6_hop_jumbo: wrong jumbo opt length/alignment %d\n",
-				    nh[optoff+1]);
+				    jumbo_hdr->tlv_len);
 		SKB_DR_SET(reason, IP_INHDR);
 		goto drop;
 	}
 
-	pkt_len = ntohl(*(__be32 *)(nh + optoff + 2));
+	pkt_len = ntohl(jumbo_hdr->jumbo_payload_len);
 	if (pkt_len <= IPV6_MAXPLEN) {
 		icmpv6_param_prob_reason(skb, ICMPV6_HDR_FIELD, optoff + 2,
 					 SKB_DROP_REASON_IP_INHDR);
@@ -1004,15 +1007,16 @@  static bool ipv6_hop_jumbo(struct sk_buff *skb, int optoff)
 
 static bool ipv6_hop_calipso(struct sk_buff *skb, int optoff)
 {
-	const unsigned char *nh = skb_network_header(skb);
+	struct hop_calipso_hdr *calipso_hdr =
+		(struct hop_calipso_hdr *)(skb_network_header(skb) + optoff);
 
-	if (nh[optoff + 1] < 8)
+	if (calipso_hdr->tlv_len < 8)
 		goto drop;
 
-	if (nh[optoff + 6] * 4 + 8 > nh[optoff + 1])
+	if (calipso_hdr->cmpt_len * 4 + 8 > calipso_hdr->tlv_len)
 		goto drop;
 
-	if (!calipso_validate(skb, nh + optoff))
+	if (!calipso_validate(skb, (const unsigned char *)calipso_hdr))
 		goto drop;
 
 	return true;