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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 19 Apr 2018 08:37:24 +0300
From:   yotam gigi <yotam.gi@...il.com>
To:     Alexander Aring <aring@...atatu.com>
Cc:     Jamal Hadi Salim <jhs@...atatu.com>, davem@...emloft.net,
        Cong Wang <xiyou.wangcong@...il.com>,
        Jiří Pírko <jiri@...nulli.us>,
        Yuval Mintz <yuvalm@...lanox.com>, netdev@...r.kernel.org,
        kernel@...atatu.com
Subject: Re: [PATCH net 2/3] net: sched: ife: handle malformed tlv length

On Thu, Apr 19, 2018 at 12:35 AM, Alexander Aring <aring@...atatu.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@...atatu.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@...il.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
>