[PATCH net 0/3] net: sched: ife: malformed ife packet fixes

[PATCH net 0/3] net: sched: ife: malformed ife packet fixes

[Alexander Aring]

As promised at netdev 2.2 tc workshop I am working on adding scapy support for
tdc testing. It is still work in progress. I will submit the patches to tdc
later (they are not in good shape yet). The good news is I have been able to
find bugs which normal packet testing would not be able to find.
With fuzzy testing I was able to craft certain malformed packets that IFE
action was not able to deal with. This patch set fixes those bugs.

Alexander Aring (3):
  net: sched: ife: signal not finding metaid
  net: sched: ife: handle malformed tlv length
  net: sched: ife: check on metadata length

 include/net/ife.h   |  3 ++-
 net/ife/ife.c       | 38 ++++++++++++++++++++++++++++++++++++--
 net/sched/act_ife.c |  9 +++++++--
 3 files changed, 45 insertions(+), 5 deletions(-)

-- 
2.11.0

[PATCH net 1/3] net: sched: ife: signal not finding metaid

[Alexander Aring]

We need to record stats for received metadata that we dont know how
to process. Have find_decode_metaid() return -ENOENT to capture this.

Signed-off-by: Alexander Aring <aring@mojatatu.com>
---
 net/sched/act_ife.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/sched/act_ife.c b/net/sched/act_ife.c
index a5994cf0512b..49b8ab551fbe 100644
--- a/net/sched/act_ife.c
+++ b/net/sched/act_ife.c
@@ -652,7 +652,7 @@ static int find_decode_metaid(struct sk_buff *skb, struct tcf_ife_info *ife,
 		}
 	}
 
-	return 0;
+	return -ENOENT;
 }
 
 static int tcf_ife_decode(struct sk_buff *skb, const struct tc_action *a,
-- 
2.11.0

[PATCH net 2/3] net: sched: ife: handle malformed tlv length

[Alexander Aring]

There is currently no handling to check on a invalid tlv length. This
patch adds such handling to avoid killing the kernel with a malformed
ife packet.

Signed-off-by: Alexander Aring <aring@mojatatu.com>
---
 include/net/ife.h   |  3 ++-
 net/ife/ife.c       | 35 +++++++++++++++++++++++++++++++++--
 net/sched/act_ife.c |  7 ++++++-
 3 files changed, 41 insertions(+), 4 deletions(-)

diff --git a/include/net/ife.h b/include/net/ife.h
index 44b9c00f7223..e117617e3c34 100644
--- a/include/net/ife.h
+++ b/include/net/ife.h
@@ -12,7 +12,8 @@
 void *ife_encode(struct sk_buff *skb, u16 metalen);
 void *ife_decode(struct sk_buff *skb, u16 *metalen);
 
-void *ife_tlv_meta_decode(void *skbdata, u16 *attrtype, u16 *dlen, u16 *totlen);
+void *ife_tlv_meta_decode(void *skbdata, const void *ifehdr_end, u16 *attrtype,
+			  u16 *dlen, u16 *totlen);
 int ife_tlv_meta_encode(void *skbdata, u16 attrtype, u16 dlen,
 			const void *dval);
 
diff --git a/net/ife/ife.c b/net/ife/ife.c
index 7d1ec76e7f43..8632d2685efb 100644
--- a/net/ife/ife.c
+++ b/net/ife/ife.c
@@ -92,12 +92,43 @@ struct meta_tlvhdr {
 	__be16 len;
 };
 
+static inline bool __ife_tlv_meta_valid(const unsigned char *skbdata,
+					const unsigned char *ifehdr_end)
+{
+	const struct meta_tlvhdr *tlv;
+	u16 tlvlen;
+
+	if (unlikely(skbdata + sizeof(*tlv) > ifehdr_end))
+		return false;
+
+	tlv = (struct meta_tlvhdr *)skbdata;
+	tlvlen = ntohs(tlv->len);
+
+	/* tlv length field is inc header, check on minimum */
+	if (tlvlen < NLA_HDRLEN)
+		return false;
+
+	/* overflow by NLA_ALIGN check */
+	if (NLA_ALIGN(tlvlen) < tlvlen)
+		return false;
+
+	if (unlikely(skbdata + NLA_ALIGN(tlvlen) > ifehdr_end))
+		return false;
+
+	return true;
+}
+
 /* Caller takes care of presenting data in network order
  */
-void *ife_tlv_meta_decode(void *skbdata, u16 *attrtype, u16 *dlen, u16 *totlen)
+void *ife_tlv_meta_decode(void *skbdata, const void *ifehdr_end, u16 *attrtype,
+			  u16 *dlen, u16 *totlen)
 {
-	struct meta_tlvhdr *tlv = (struct meta_tlvhdr *) skbdata;
+	struct meta_tlvhdr *tlv;
+
+	if (!__ife_tlv_meta_valid(skbdata, ifehdr_end))
+		return NULL;
 
+	tlv = (struct meta_tlvhdr *)skbdata;
 	*dlen = ntohs(tlv->len) - NLA_HDRLEN;
 	*attrtype = ntohs(tlv->type);
 
diff --git a/net/sched/act_ife.c b/net/sched/act_ife.c
index 49b8ab551fbe..8527cfdc446d 100644
--- a/net/sched/act_ife.c
+++ b/net/sched/act_ife.c
@@ -682,7 +682,12 @@ static int tcf_ife_decode(struct sk_buff *skb, const struct tc_action *a,
 		u16 mtype;
 		u16 dlen;
 
-		curr_data = ife_tlv_meta_decode(tlv_data, &mtype, &dlen, NULL);
+		curr_data = ife_tlv_meta_decode(tlv_data, ifehdr_end, &mtype,
+						&dlen, NULL);
+		if (!curr_data) {
+			qstats_drop_inc(this_cpu_ptr(ife->common.cpu_qstats));
+			return TC_ACT_SHOT;
+		}
 
 		if (find_decode_metaid(skb, ife, mtype, dlen, curr_data)) {
 			/* abuse overlimits to count when we receive metadata
-- 
2.11.0

[PATCH net 3/3] net: sched: ife: check on metadata length

[Alexander Aring]

This patch checks if sk buffer is available to dererence ife header. If
not then NULL will returned to signal an malformed ife packet. This
avoids to crashing the kernel from outside.

Signed-off-by: Alexander Aring <aring@mojatatu.com>
---
 net/ife/ife.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/net/ife/ife.c b/net/ife/ife.c
index 8632d2685efb..7c100034fbee 100644
--- a/net/ife/ife.c
+++ b/net/ife/ife.c
@@ -70,6 +70,9 @@ void *ife_decode(struct sk_buff *skb, u16 *metalen)
 	u16 ifehdrln;
 
 	ifehdr = (struct ifeheadr *) (skb->data + skb->dev->hard_header_len);
+	if (skb->len < skb->dev->hard_header_len + IFE_METAHDRLEN)
+		return NULL;
+
 	ifehdrln = ntohs(ifehdr->metalen);
 	total_pull = skb->dev->hard_header_len + ifehdrln;
 
-- 
2.11.0

Re: [PATCH net 1/3] net: sched: ife: signal not finding metaid

[yotam gigi]

On Thu, Apr 19, 2018 at 12:35 AM, Alexander Aring <aring@mojatatu.com> wrote:
> We need to record stats for received metadata that we dont know how
> to process. Have find_decode_metaid() return -ENOENT to capture this.

Agree.

>
> Signed-off-by: Alexander Aring <aring@mojatatu.com>

Reviewed-by: Yotam Gigi <yotam.gi@gmail.com>

> ---
>  net/sched/act_ife.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/sched/act_ife.c b/net/sched/act_ife.c
> index a5994cf0512b..49b8ab551fbe 100644
> --- a/net/sched/act_ife.c
> +++ b/net/sched/act_ife.c
> @@ -652,7 +652,7 @@ static int find_decode_metaid(struct sk_buff *skb, struct tcf_ife_info *ife,
>                 }
>         }
>
> -       return 0;
> +       return -ENOENT;
>  }
>
>  static int tcf_ife_decode(struct sk_buff *skb, const struct tc_action *a,
> --
> 2.11.0
>

Re: [PATCH net 2/3] net: sched: ife: handle malformed tlv length

[yotam gigi]

On Thu, Apr 19, 2018 at 12:35 AM, Alexander Aring <aring@mojatatu.com> wrote:
> There is currently no handling to check on a invalid tlv length. This
> patch adds such handling to avoid killing the kernel with a malformed
> ife packet.

That's very important. Thanks for that!

>
> Signed-off-by: Alexander Aring <aring@mojatatu.com>
> ---
>  include/net/ife.h   |  3 ++-
>  net/ife/ife.c       | 35 +++++++++++++++++++++++++++++++++--
>  net/sched/act_ife.c |  7 ++++++-
>  3 files changed, 41 insertions(+), 4 deletions(-)
>
> diff --git a/include/net/ife.h b/include/net/ife.h
> index 44b9c00f7223..e117617e3c34 100644
> --- a/include/net/ife.h
> +++ b/include/net/ife.h
> @@ -12,7 +12,8 @@
>  void *ife_encode(struct sk_buff *skb, u16 metalen);
>  void *ife_decode(struct sk_buff *skb, u16 *metalen);
>
> -void *ife_tlv_meta_decode(void *skbdata, u16 *attrtype, u16 *dlen, u16 *totlen);
> +void *ife_tlv_meta_decode(void *skbdata, const void *ifehdr_end, u16 *attrtype,
> +                         u16 *dlen, u16 *totlen);
>  int ife_tlv_meta_encode(void *skbdata, u16 attrtype, u16 dlen,
>                         const void *dval);
>
> diff --git a/net/ife/ife.c b/net/ife/ife.c
> index 7d1ec76e7f43..8632d2685efb 100644
> --- a/net/ife/ife.c
> +++ b/net/ife/ife.c
> @@ -92,12 +92,43 @@ struct meta_tlvhdr {
>         __be16 len;
>  };
>
> +static inline bool __ife_tlv_meta_valid(const unsigned char *skbdata,
> +                                       const unsigned char *ifehdr_end)
> +{
> +       const struct meta_tlvhdr *tlv;
> +       u16 tlvlen;
> +
> +       if (unlikely(skbdata + sizeof(*tlv) > ifehdr_end))
> +               return false;
> +
> +       tlv = (struct meta_tlvhdr *)skbdata;
> +       tlvlen = ntohs(tlv->len);
> +
> +       /* tlv length field is inc header, check on minimum */
> +       if (tlvlen < NLA_HDRLEN)
> +               return false;
> +
> +       /* overflow by NLA_ALIGN check */
> +       if (NLA_ALIGN(tlvlen) < tlvlen)
> +               return false;
> +
> +       if (unlikely(skbdata + NLA_ALIGN(tlvlen) > ifehdr_end))
> +               return false;
> +
> +       return true;
> +}
> +
>  /* Caller takes care of presenting data in network order
>   */
> -void *ife_tlv_meta_decode(void *skbdata, u16 *attrtype, u16 *dlen, u16 *totlen)
> +void *ife_tlv_meta_decode(void *skbdata, const void *ifehdr_end, u16 *attrtype,
> +                         u16 *dlen, u16 *totlen)

Now it is not critical, but it looks a bit odd to me: the function ife_decode
returns "metalen" - maybe it is better to change it too to return ifehdr_end?
If not, the caller has to calculate it himself for no particular reason.
Having both metalen and ifehdr_end is redundant, so we should stick to only
one of these.

Other than that,
Reviewed-by: Yotam Gigi <yotam.gi@gmail.com>

>  {
> -       struct meta_tlvhdr *tlv = (struct meta_tlvhdr *) skbdata;
> +       struct meta_tlvhdr *tlv;
> +
> +       if (!__ife_tlv_meta_valid(skbdata, ifehdr_end))
> +               return NULL;
>
> +       tlv = (struct meta_tlvhdr *)skbdata;
>         *dlen = ntohs(tlv->len) - NLA_HDRLEN;
>         *attrtype = ntohs(tlv->type);
>
> diff --git a/net/sched/act_ife.c b/net/sched/act_ife.c
> index 49b8ab551fbe..8527cfdc446d 100644
> --- a/net/sched/act_ife.c
> +++ b/net/sched/act_ife.c
> @@ -682,7 +682,12 @@ static int tcf_ife_decode(struct sk_buff *skb, const struct tc_action *a,
>                 u16 mtype;
>                 u16 dlen;
>
> -               curr_data = ife_tlv_meta_decode(tlv_data, &mtype, &dlen, NULL);
> +               curr_data = ife_tlv_meta_decode(tlv_data, ifehdr_end, &mtype,
> +                                               &dlen, NULL);
> +               if (!curr_data) {
> +                       qstats_drop_inc(this_cpu_ptr(ife->common.cpu_qstats));
> +                       return TC_ACT_SHOT;
> +               }
>
>                 if (find_decode_metaid(skb, ife, mtype, dlen, curr_data)) {
>                         /* abuse overlimits to count when we receive metadata
> --
> 2.11.0
>

Re: [PATCH net 3/3] net: sched: ife: check on metadata length

[yotam gigi]

On Thu, Apr 19, 2018 at 12:35 AM, Alexander Aring <aring@mojatatu.com> wrote:
> This patch checks if sk buffer is available to dererence ife header. If
> not then NULL will returned to signal an malformed ife packet. This
> avoids to crashing the kernel from outside.
>
> Signed-off-by: Alexander Aring <aring@mojatatu.com>

Reviewed-by: Yotam Gigi <yotam.gi@gmail.com>

> ---
>  net/ife/ife.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/net/ife/ife.c b/net/ife/ife.c
> index 8632d2685efb..7c100034fbee 100644
> --- a/net/ife/ife.c
> +++ b/net/ife/ife.c
> @@ -70,6 +70,9 @@ void *ife_decode(struct sk_buff *skb, u16 *metalen)
>         u16 ifehdrln;
>
>         ifehdr = (struct ifeheadr *) (skb->data + skb->dev->hard_header_len);
> +       if (skb->len < skb->dev->hard_header_len + IFE_METAHDRLEN)
> +               return NULL;
> +
>         ifehdrln = ntohs(ifehdr->metalen);
>         total_pull = skb->dev->hard_header_len + ifehdrln;
>
> --
> 2.11.0
>

Re: [PATCH net 1/3] net: sched: ife: signal not finding metaid

[Jamal Hadi Salim]

On 19/04/18 01:37 AM, yotam gigi wrote:
> On Thu, Apr 19, 2018 at 12:35 AM, Alexander Aring <aring@mojatatu.com> wrote:
>> We need to record stats for received metadata that we dont know how
>> to process. Have find_decode_metaid() return -ENOENT to capture this.
> 
> Agree.
> 
>>
>> Signed-off-by: Alexander Aring <aring@mojatatu.com>
> 
> Reviewed-by: Yotam Gigi <yotam.gi@gmail.com>

Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>

Re: [PATCH net 2/3] net: sched: ife: handle malformed tlv length

[Jamal Hadi Salim]

On 19/04/18 01:37 AM, yotam gigi wrote:
> On Thu, Apr 19, 2018 at 12:35 AM, Alexander Aring <aring@mojatatu.com> wrote:
>> There is currently no handling to check on a invalid tlv length. This
>> patch adds such handling to avoid killing the kernel with a malformed
>> ife packet.
> 
> That's very important. Thanks for that!
> 
>>
>> Signed-off-by: Alexander Aring <aring@mojatatu.com>

Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>

cheers,
jamal

Re: [PATCH net 3/3] net: sched: ife: check on metadata length

[Jamal Hadi Salim]

On 19/04/18 01:37 AM, yotam gigi wrote:
> On Thu, Apr 19, 2018 at 12:35 AM, Alexander Aring <aring@mojatatu.com> wrote:
>> This patch checks if sk buffer is available to dererence ife header. If
>> not then NULL will returned to signal an malformed ife packet. This
>> avoids to crashing the kernel from outside.
>>
>> Signed-off-by: Alexander Aring <aring@mojatatu.com>
> 
> Reviewed-by: Yotam Gigi <yotam.gi@gmail.com>
>

Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>

cheers,
jamal

Re: [PATCH net 2/3] net: sched: ife: handle malformed tlv length

[David Miller]

From: Alexander Aring <aring@mojatatu.com>
Date: Wed, 18 Apr 2018 17:35:33 -0400

> @@ -92,12 +92,43 @@ struct meta_tlvhdr {
>  	__be16 len;
>  };
>  
> +static inline bool __ife_tlv_meta_valid(const unsigned char *skbdata,
> +					const unsigned char *ifehdr_end)
> +{

Please do not use inline in foo.c files, let the compiler decide.