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

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(-)

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
>

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

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.