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