* [PATCH net 0/3] net: sched: ife: malformed ife packet fixes
@ 2018-04-18 21:35 Alexander Aring
2018-04-18 21:35 ` [PATCH net 1/3] net: sched: ife: signal not finding metaid Alexander Aring
` (2 more replies)
0 siblings, 3 replies; 11+ messages in thread
From: Alexander Aring @ 2018-04-18 21:35 UTC (permalink / raw)
To: yotam.gi
Cc: jhs, davem, xiyou.wangcong, jiri, yuvalm, netdev, kernel,
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
^ permalink raw reply [flat|nested] 11+ messages in thread* [PATCH net 1/3] net: sched: ife: signal not finding metaid 2018-04-18 21:35 [PATCH net 0/3] net: sched: ife: malformed ife packet fixes Alexander Aring @ 2018-04-18 21:35 ` Alexander Aring 2018-04-19 5:37 ` yotam gigi 2018-04-18 21:35 ` [PATCH net 2/3] net: sched: ife: handle malformed tlv length Alexander Aring 2018-04-18 21:35 ` [PATCH net 3/3] net: sched: ife: check on metadata length Alexander Aring 2 siblings, 1 reply; 11+ messages in thread From: Alexander Aring @ 2018-04-18 21:35 UTC (permalink / raw) To: yotam.gi Cc: jhs, davem, xiyou.wangcong, jiri, yuvalm, netdev, kernel, 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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH net 1/3] net: sched: ife: signal not finding metaid 2018-04-18 21:35 ` [PATCH net 1/3] net: sched: ife: signal not finding metaid Alexander Aring @ 2018-04-19 5:37 ` yotam gigi 2018-04-19 12:08 ` Jamal Hadi Salim 0 siblings, 1 reply; 11+ messages in thread From: yotam gigi @ 2018-04-19 5:37 UTC (permalink / raw) To: Alexander Aring Cc: Jamal Hadi Salim, davem, Cong Wang, Jiří Pírko, Yuval Mintz, netdev, kernel 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 > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net 1/3] net: sched: ife: signal not finding metaid 2018-04-19 5:37 ` yotam gigi @ 2018-04-19 12:08 ` Jamal Hadi Salim 0 siblings, 0 replies; 11+ messages in thread From: Jamal Hadi Salim @ 2018-04-19 12:08 UTC (permalink / raw) To: yotam gigi, Alexander Aring Cc: davem, Cong Wang, Jiří Pírko, Yuval Mintz, netdev, kernel 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> ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH net 2/3] net: sched: ife: handle malformed tlv length 2018-04-18 21:35 [PATCH net 0/3] net: sched: ife: malformed ife packet fixes Alexander Aring 2018-04-18 21:35 ` [PATCH net 1/3] net: sched: ife: signal not finding metaid Alexander Aring @ 2018-04-18 21:35 ` Alexander Aring 2018-04-19 5:37 ` yotam gigi 2018-04-19 19:30 ` David Miller 2018-04-18 21:35 ` [PATCH net 3/3] net: sched: ife: check on metadata length Alexander Aring 2 siblings, 2 replies; 11+ messages in thread From: Alexander Aring @ 2018-04-18 21:35 UTC (permalink / raw) To: yotam.gi Cc: jhs, davem, xiyou.wangcong, jiri, yuvalm, netdev, kernel, 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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH net 2/3] net: sched: ife: handle malformed tlv length 2018-04-18 21:35 ` [PATCH net 2/3] net: sched: ife: handle malformed tlv length Alexander Aring @ 2018-04-19 5:37 ` yotam gigi 2018-04-19 12:09 ` Jamal Hadi Salim 2018-04-19 19:30 ` David Miller 1 sibling, 1 reply; 11+ messages in thread From: yotam gigi @ 2018-04-19 5:37 UTC (permalink / raw) To: Alexander Aring Cc: Jamal Hadi Salim, davem, Cong Wang, Jiří Pírko, Yuval Mintz, netdev, kernel 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 > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net 2/3] net: sched: ife: handle malformed tlv length 2018-04-19 5:37 ` yotam gigi @ 2018-04-19 12:09 ` Jamal Hadi Salim 0 siblings, 0 replies; 11+ messages in thread From: Jamal Hadi Salim @ 2018-04-19 12:09 UTC (permalink / raw) To: yotam gigi, Alexander Aring Cc: davem, Cong Wang, Jiří Pírko, Yuval Mintz, netdev, kernel 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net 2/3] net: sched: ife: handle malformed tlv length 2018-04-18 21:35 ` [PATCH net 2/3] net: sched: ife: handle malformed tlv length Alexander Aring 2018-04-19 5:37 ` yotam gigi @ 2018-04-19 19:30 ` David Miller 1 sibling, 0 replies; 11+ messages in thread From: David Miller @ 2018-04-19 19:30 UTC (permalink / raw) To: aring; +Cc: yotam.gi, jhs, xiyou.wangcong, jiri, yuvalm, netdev, kernel 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. ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH net 3/3] net: sched: ife: check on metadata length 2018-04-18 21:35 [PATCH net 0/3] net: sched: ife: malformed ife packet fixes Alexander Aring 2018-04-18 21:35 ` [PATCH net 1/3] net: sched: ife: signal not finding metaid Alexander Aring 2018-04-18 21:35 ` [PATCH net 2/3] net: sched: ife: handle malformed tlv length Alexander Aring @ 2018-04-18 21:35 ` Alexander Aring 2018-04-19 5:37 ` yotam gigi 2 siblings, 1 reply; 11+ messages in thread From: Alexander Aring @ 2018-04-18 21:35 UTC (permalink / raw) To: yotam.gi Cc: jhs, davem, xiyou.wangcong, jiri, yuvalm, netdev, kernel, 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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH net 3/3] net: sched: ife: check on metadata length 2018-04-18 21:35 ` [PATCH net 3/3] net: sched: ife: check on metadata length Alexander Aring @ 2018-04-19 5:37 ` yotam gigi 2018-04-19 12:10 ` Jamal Hadi Salim 0 siblings, 1 reply; 11+ messages in thread From: yotam gigi @ 2018-04-19 5:37 UTC (permalink / raw) To: Alexander Aring Cc: Jamal Hadi Salim, davem, Cong Wang, Jiří Pírko, Yuval Mintz, netdev, kernel 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 > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net 3/3] net: sched: ife: check on metadata length 2018-04-19 5:37 ` yotam gigi @ 2018-04-19 12:10 ` Jamal Hadi Salim 0 siblings, 0 replies; 11+ messages in thread From: Jamal Hadi Salim @ 2018-04-19 12:10 UTC (permalink / raw) To: yotam gigi, Alexander Aring Cc: davem, Cong Wang, Jiří Pírko, Yuval Mintz, netdev, kernel 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2018-04-19 19:30 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-04-18 21:35 [PATCH net 0/3] net: sched: ife: malformed ife packet fixes Alexander Aring 2018-04-18 21:35 ` [PATCH net 1/3] net: sched: ife: signal not finding metaid Alexander Aring 2018-04-19 5:37 ` yotam gigi 2018-04-19 12:08 ` Jamal Hadi Salim 2018-04-18 21:35 ` [PATCH net 2/3] net: sched: ife: handle malformed tlv length Alexander Aring 2018-04-19 5:37 ` yotam gigi 2018-04-19 12:09 ` Jamal Hadi Salim 2018-04-19 19:30 ` David Miller 2018-04-18 21:35 ` [PATCH net 3/3] net: sched: ife: check on metadata length Alexander Aring 2018-04-19 5:37 ` yotam gigi 2018-04-19 12:10 ` Jamal Hadi Salim
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.