* [PATCH net V2] net/sched: pedit: make sure that offset is valid @ 2016-11-28 10:56 Amir Vadai 2016-11-29 2:32 ` zhuyj 2016-11-30 0:46 ` David Miller 0 siblings, 2 replies; 6+ messages in thread From: Amir Vadai @ 2016-11-28 10:56 UTC (permalink / raw) To: David S. Miller Cc: netdev, Cong Wang, Jamal Hadi Salim, Or Gerlitz, Hadar Har-Zion, Jiri Pirko, Amir Vadai Add a validation function to make sure offset is valid: 1. Not below skb head (could happen when offset is negative). 2. Validate both 'offset' and 'at'. Signed-off-by: Amir Vadai <amir@vadai.me> --- Hi Dave, Please pull to -stable branches. Changes from V0: - Add a validation to the 'at' value (this is used as an offset too) - Instead of validating the output of skb_header_pointer(), make sure that the offset is good before calling it. Thanks, Amir net/sched/act_pedit.c | 24 ++++++++++++++++++++---- 1 file changed, 20 insertions(+), 4 deletions(-) diff --git a/net/sched/act_pedit.c b/net/sched/act_pedit.c index b54d56d4959b..cf9b2fe8eac6 100644 --- a/net/sched/act_pedit.c +++ b/net/sched/act_pedit.c @@ -108,6 +108,17 @@ static void tcf_pedit_cleanup(struct tc_action *a, int bind) kfree(keys); } +static bool offset_valid(struct sk_buff *skb, int offset) +{ + if (offset > 0 && offset > skb->len) + return false; + + if (offset < 0 && -offset > skb_headroom(skb)) + return false; + + return true; +} + static int tcf_pedit(struct sk_buff *skb, const struct tc_action *a, struct tcf_result *res) { @@ -134,6 +145,11 @@ static int tcf_pedit(struct sk_buff *skb, const struct tc_action *a, if (tkey->offmask) { char *d, _d; + if (!offset_valid(skb, off + tkey->at)) { + pr_info("tc filter pedit 'at' offset %d out of bounds\n", + off + tkey->at); + goto bad; + } d = skb_header_pointer(skb, off + tkey->at, 1, &_d); if (!d) @@ -146,10 +162,10 @@ static int tcf_pedit(struct sk_buff *skb, const struct tc_action *a, " offset must be on 32 bit boundaries\n"); goto bad; } - if (offset > 0 && offset > skb->len) { - pr_info("tc filter pedit" - " offset %d can't exceed pkt length %d\n", - offset, skb->len); + + if (!offset_valid(skb, off + offset)) { + pr_info("tc filter pedit offset %d out of bounds\n", + offset); goto bad; } -- 2.10.2 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH net V2] net/sched: pedit: make sure that offset is valid 2016-11-28 10:56 [PATCH net V2] net/sched: pedit: make sure that offset is valid Amir Vadai @ 2016-11-29 2:32 ` zhuyj 2016-11-29 7:14 ` Amir Vadai 2016-11-30 0:46 ` David Miller 1 sibling, 1 reply; 6+ messages in thread From: zhuyj @ 2016-11-29 2:32 UTC (permalink / raw) To: Amir Vadai Cc: David S. Miller, netdev, Cong Wang, Jamal Hadi Salim, Or Gerlitz, Hadar Har-Zion, Jiri Pirko + if (offset > 0 && offset > skb->len) offset > skb->len is enough? On Mon, Nov 28, 2016 at 6:56 PM, Amir Vadai <amir@vadai.me> wrote: > Add a validation function to make sure offset is valid: > 1. Not below skb head (could happen when offset is negative). > 2. Validate both 'offset' and 'at'. > > Signed-off-by: Amir Vadai <amir@vadai.me> > --- > Hi Dave, > > Please pull to -stable branches. > > Changes from V0: > - Add a validation to the 'at' value (this is used as an offset too) > - Instead of validating the output of skb_header_pointer(), make sure that the > offset is good before calling it. > > Thanks, > Amir > net/sched/act_pedit.c | 24 ++++++++++++++++++++---- > 1 file changed, 20 insertions(+), 4 deletions(-) > > diff --git a/net/sched/act_pedit.c b/net/sched/act_pedit.c > index b54d56d4959b..cf9b2fe8eac6 100644 > --- a/net/sched/act_pedit.c > +++ b/net/sched/act_pedit.c > @@ -108,6 +108,17 @@ static void tcf_pedit_cleanup(struct tc_action *a, int bind) > kfree(keys); > } > > +static bool offset_valid(struct sk_buff *skb, int offset) > +{ > + if (offset > 0 && offset > skb->len) > + return false; > + > + if (offset < 0 && -offset > skb_headroom(skb)) > + return false; > + > + return true; > +} > + > static int tcf_pedit(struct sk_buff *skb, const struct tc_action *a, > struct tcf_result *res) > { > @@ -134,6 +145,11 @@ static int tcf_pedit(struct sk_buff *skb, const struct tc_action *a, > if (tkey->offmask) { > char *d, _d; > > + if (!offset_valid(skb, off + tkey->at)) { > + pr_info("tc filter pedit 'at' offset %d out of bounds\n", > + off + tkey->at); > + goto bad; > + } > d = skb_header_pointer(skb, off + tkey->at, 1, > &_d); > if (!d) > @@ -146,10 +162,10 @@ static int tcf_pedit(struct sk_buff *skb, const struct tc_action *a, > " offset must be on 32 bit boundaries\n"); > goto bad; > } > - if (offset > 0 && offset > skb->len) { > - pr_info("tc filter pedit" > - " offset %d can't exceed pkt length %d\n", > - offset, skb->len); > + > + if (!offset_valid(skb, off + offset)) { > + pr_info("tc filter pedit offset %d out of bounds\n", > + offset); > goto bad; > } > > -- > 2.10.2 > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net V2] net/sched: pedit: make sure that offset is valid 2016-11-29 2:32 ` zhuyj @ 2016-11-29 7:14 ` Amir Vadai 2016-11-29 9:27 ` zhuyj 0 siblings, 1 reply; 6+ messages in thread From: Amir Vadai @ 2016-11-29 7:14 UTC (permalink / raw) To: zhuyj Cc: David S. Miller, netdev, Cong Wang, Jamal Hadi Salim, Or Gerlitz, Hadar Har-Zion, Jiri Pirko On Tue, Nov 29, 2016 at 10:32:05AM +0800, zhuyj wrote: > + if (offset > 0 && offset > skb->len) > > offset > skb->len is enough? offset is signed and skb->len is unsigned. Therefore for example if offset=-1 and skb->len=10, the actual comparison is 0xff...>10 > > On Mon, Nov 28, 2016 at 6:56 PM, Amir Vadai <amir@vadai.me> wrote: > > Add a validation function to make sure offset is valid: > > 1. Not below skb head (could happen when offset is negative). > > 2. Validate both 'offset' and 'at'. > > > > Signed-off-by: Amir Vadai <amir@vadai.me> > > --- > > Hi Dave, > > > > Please pull to -stable branches. > > > > Changes from V0: > > - Add a validation to the 'at' value (this is used as an offset too) > > - Instead of validating the output of skb_header_pointer(), make sure that the > > offset is good before calling it. > > > > Thanks, > > Amir > > net/sched/act_pedit.c | 24 ++++++++++++++++++++---- > > 1 file changed, 20 insertions(+), 4 deletions(-) > > > > diff --git a/net/sched/act_pedit.c b/net/sched/act_pedit.c > > index b54d56d4959b..cf9b2fe8eac6 100644 > > --- a/net/sched/act_pedit.c > > +++ b/net/sched/act_pedit.c > > @@ -108,6 +108,17 @@ static void tcf_pedit_cleanup(struct tc_action *a, int bind) > > kfree(keys); > > } > > > > +static bool offset_valid(struct sk_buff *skb, int offset) > > +{ > > + if (offset > 0 && offset > skb->len) > > + return false; > > + > > + if (offset < 0 && -offset > skb_headroom(skb)) > > + return false; > > + > > + return true; > > +} > > + > > static int tcf_pedit(struct sk_buff *skb, const struct tc_action *a, > > struct tcf_result *res) > > { > > @@ -134,6 +145,11 @@ static int tcf_pedit(struct sk_buff *skb, const struct tc_action *a, > > if (tkey->offmask) { > > char *d, _d; > > > > + if (!offset_valid(skb, off + tkey->at)) { > > + pr_info("tc filter pedit 'at' offset %d out of bounds\n", > > + off + tkey->at); > > + goto bad; > > + } > > d = skb_header_pointer(skb, off + tkey->at, 1, > > &_d); > > if (!d) > > @@ -146,10 +162,10 @@ static int tcf_pedit(struct sk_buff *skb, const struct tc_action *a, > > " offset must be on 32 bit boundaries\n"); > > goto bad; > > } > > - if (offset > 0 && offset > skb->len) { > > - pr_info("tc filter pedit" > > - " offset %d can't exceed pkt length %d\n", > > - offset, skb->len); > > + > > + if (!offset_valid(skb, off + offset)) { > > + pr_info("tc filter pedit offset %d out of bounds\n", > > + offset); > > goto bad; > > } > > > > -- > > 2.10.2 > > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net V2] net/sched: pedit: make sure that offset is valid 2016-11-29 7:14 ` Amir Vadai @ 2016-11-29 9:27 ` zhuyj 2016-11-29 11:03 ` Amir Vadai 0 siblings, 1 reply; 6+ messages in thread From: zhuyj @ 2016-11-29 9:27 UTC (permalink / raw) To: Amir Vadai Cc: David S. Miller, netdev, Cong Wang, Jamal Hadi Salim, Or Gerlitz, Hadar Har-Zion, Jiri Pirko Thanks a lot. When will offset become -1? On Tue, Nov 29, 2016 at 3:14 PM, Amir Vadai <amir@vadai.me> wrote: > On Tue, Nov 29, 2016 at 10:32:05AM +0800, zhuyj wrote: >> + if (offset > 0 && offset > skb->len) >> >> offset > skb->len is enough? > offset is signed and skb->len is unsigned. Therefore for example if > offset=-1 and skb->len=10, the actual comparison is 0xff...>10 > >> >> On Mon, Nov 28, 2016 at 6:56 PM, Amir Vadai <amir@vadai.me> wrote: >> > Add a validation function to make sure offset is valid: >> > 1. Not below skb head (could happen when offset is negative). >> > 2. Validate both 'offset' and 'at'. >> > >> > Signed-off-by: Amir Vadai <amir@vadai.me> >> > --- >> > Hi Dave, >> > >> > Please pull to -stable branches. >> > >> > Changes from V0: >> > - Add a validation to the 'at' value (this is used as an offset too) >> > - Instead of validating the output of skb_header_pointer(), make sure that the >> > offset is good before calling it. >> > >> > Thanks, >> > Amir >> > net/sched/act_pedit.c | 24 ++++++++++++++++++++---- >> > 1 file changed, 20 insertions(+), 4 deletions(-) >> > >> > diff --git a/net/sched/act_pedit.c b/net/sched/act_pedit.c >> > index b54d56d4959b..cf9b2fe8eac6 100644 >> > --- a/net/sched/act_pedit.c >> > +++ b/net/sched/act_pedit.c >> > @@ -108,6 +108,17 @@ static void tcf_pedit_cleanup(struct tc_action *a, int bind) >> > kfree(keys); >> > } >> > >> > +static bool offset_valid(struct sk_buff *skb, int offset) >> > +{ >> > + if (offset > 0 && offset > skb->len) >> > + return false; >> > + >> > + if (offset < 0 && -offset > skb_headroom(skb)) >> > + return false; >> > + >> > + return true; >> > +} >> > + >> > static int tcf_pedit(struct sk_buff *skb, const struct tc_action *a, >> > struct tcf_result *res) >> > { >> > @@ -134,6 +145,11 @@ static int tcf_pedit(struct sk_buff *skb, const struct tc_action *a, >> > if (tkey->offmask) { >> > char *d, _d; >> > >> > + if (!offset_valid(skb, off + tkey->at)) { >> > + pr_info("tc filter pedit 'at' offset %d out of bounds\n", >> > + off + tkey->at); >> > + goto bad; >> > + } >> > d = skb_header_pointer(skb, off + tkey->at, 1, >> > &_d); >> > if (!d) >> > @@ -146,10 +162,10 @@ static int tcf_pedit(struct sk_buff *skb, const struct tc_action *a, >> > " offset must be on 32 bit boundaries\n"); >> > goto bad; >> > } >> > - if (offset > 0 && offset > skb->len) { >> > - pr_info("tc filter pedit" >> > - " offset %d can't exceed pkt length %d\n", >> > - offset, skb->len); >> > + >> > + if (!offset_valid(skb, off + offset)) { >> > + pr_info("tc filter pedit offset %d out of bounds\n", >> > + offset); >> > goto bad; >> > } >> > >> > -- >> > 2.10.2 >> > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net V2] net/sched: pedit: make sure that offset is valid 2016-11-29 9:27 ` zhuyj @ 2016-11-29 11:03 ` Amir Vadai 0 siblings, 0 replies; 6+ messages in thread From: Amir Vadai @ 2016-11-29 11:03 UTC (permalink / raw) To: zhuyj Cc: David S. Miller, netdev, Cong Wang, Jamal Hadi Salim, Or Gerlitz, Hadar Har-Zion, Jiri Pirko On Tue, Nov 29, 2016 at 05:27:05PM +0800, zhuyj wrote: > Thanks a lot. > When will offset become -1? offset is supplied by userspace. For example iproute2 tc tool. It is valid to supply a negative value, since offset is relative to the networking layer, one might want to edit a MAC layer field. Please use bottom posting when replying to mails... > > On Tue, Nov 29, 2016 at 3:14 PM, Amir Vadai <amir@vadai.me> wrote: > > On Tue, Nov 29, 2016 at 10:32:05AM +0800, zhuyj wrote: > >> + if (offset > 0 && offset > skb->len) > >> > >> offset > skb->len is enough? > > offset is signed and skb->len is unsigned. Therefore for example if > > offset=-1 and skb->len=10, the actual comparison is 0xff...>10 > > > >> > >> On Mon, Nov 28, 2016 at 6:56 PM, Amir Vadai <amir@vadai.me> wrote: > >> > Add a validation function to make sure offset is valid: > >> > 1. Not below skb head (could happen when offset is negative). > >> > 2. Validate both 'offset' and 'at'. > >> > > >> > Signed-off-by: Amir Vadai <amir@vadai.me> > >> > --- > >> > Hi Dave, > >> > > >> > Please pull to -stable branches. > >> > > >> > Changes from V0: > >> > - Add a validation to the 'at' value (this is used as an offset too) > >> > - Instead of validating the output of skb_header_pointer(), make sure that the > >> > offset is good before calling it. > >> > > >> > Thanks, > >> > Amir > >> > net/sched/act_pedit.c | 24 ++++++++++++++++++++---- > >> > 1 file changed, 20 insertions(+), 4 deletions(-) > >> > > >> > diff --git a/net/sched/act_pedit.c b/net/sched/act_pedit.c > >> > index b54d56d4959b..cf9b2fe8eac6 100644 > >> > --- a/net/sched/act_pedit.c > >> > +++ b/net/sched/act_pedit.c > >> > @@ -108,6 +108,17 @@ static void tcf_pedit_cleanup(struct tc_action *a, int bind) > >> > kfree(keys); > >> > } > >> > > >> > +static bool offset_valid(struct sk_buff *skb, int offset) > >> > +{ > >> > + if (offset > 0 && offset > skb->len) > >> > + return false; > >> > + > >> > + if (offset < 0 && -offset > skb_headroom(skb)) > >> > + return false; > >> > + > >> > + return true; > >> > +} > >> > + > >> > static int tcf_pedit(struct sk_buff *skb, const struct tc_action *a, > >> > struct tcf_result *res) > >> > { > >> > @@ -134,6 +145,11 @@ static int tcf_pedit(struct sk_buff *skb, const struct tc_action *a, > >> > if (tkey->offmask) { > >> > char *d, _d; > >> > > >> > + if (!offset_valid(skb, off + tkey->at)) { > >> > + pr_info("tc filter pedit 'at' offset %d out of bounds\n", > >> > + off + tkey->at); > >> > + goto bad; > >> > + } > >> > d = skb_header_pointer(skb, off + tkey->at, 1, > >> > &_d); > >> > if (!d) > >> > @@ -146,10 +162,10 @@ static int tcf_pedit(struct sk_buff *skb, const struct tc_action *a, > >> > " offset must be on 32 bit boundaries\n"); > >> > goto bad; > >> > } > >> > - if (offset > 0 && offset > skb->len) { > >> > - pr_info("tc filter pedit" > >> > - " offset %d can't exceed pkt length %d\n", > >> > - offset, skb->len); > >> > + > >> > + if (!offset_valid(skb, off + offset)) { > >> > + pr_info("tc filter pedit offset %d out of bounds\n", > >> > + offset); > >> > goto bad; > >> > } > >> > > >> > -- > >> > 2.10.2 > >> > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net V2] net/sched: pedit: make sure that offset is valid 2016-11-28 10:56 [PATCH net V2] net/sched: pedit: make sure that offset is valid Amir Vadai 2016-11-29 2:32 ` zhuyj @ 2016-11-30 0:46 ` David Miller 1 sibling, 0 replies; 6+ messages in thread From: David Miller @ 2016-11-30 0:46 UTC (permalink / raw) To: amir; +Cc: netdev, xiyou.wangcong, jhs, ogerlitz, hadarh, jiri From: Amir Vadai <amir@vadai.me> Date: Mon, 28 Nov 2016 12:56:40 +0200 > Add a validation function to make sure offset is valid: > 1. Not below skb head (could happen when offset is negative). > 2. Validate both 'offset' and 'at'. > > Signed-off-by: Amir Vadai <amir@vadai.me> Applied and queued up for -stable, thanks. ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-11-30 0:46 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-11-28 10:56 [PATCH net V2] net/sched: pedit: make sure that offset is valid Amir Vadai 2016-11-29 2:32 ` zhuyj 2016-11-29 7:14 ` Amir Vadai 2016-11-29 9:27 ` zhuyj 2016-11-29 11:03 ` Amir Vadai 2016-11-30 0:46 ` David Miller
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).