* [PATCH net] net/sched: act_mpls: ensure LSE is pullable before reading it
@ 2020-11-18 16:36 Davide Caratti
2020-11-18 16:47 ` Marcelo Ricardo Leitner
2020-11-20 19:24 ` Jakub Kicinski
0 siblings, 2 replies; 5+ messages in thread
From: Davide Caratti @ 2020-11-18 16:36 UTC (permalink / raw)
To: Jamal Hadi Salim, Cong Wang, Jiri Pirko, netdev, Jakub Kicinski
Cc: gnault, marcelo.leitner
when 'act_mpls' is used to mangle the LSE, the current value is read from
the packet with mpls_hdr(): ensure that the label is contained in the skb
"linear" area.
Found by code inspection.
Fixes: 2a2ea50870ba ("net: sched: add mpls manipulation actions to TC")
Signed-off-by: Davide Caratti <dcaratti@redhat.com>
---
net/sched/act_mpls.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/net/sched/act_mpls.c b/net/sched/act_mpls.c
index 5c7456e5b5cf..03138ad59e9b 100644
--- a/net/sched/act_mpls.c
+++ b/net/sched/act_mpls.c
@@ -105,6 +105,9 @@ static int tcf_mpls_act(struct sk_buff *skb, const struct tc_action *a,
goto drop;
break;
case TCA_MPLS_ACT_MODIFY:
+ if (!pskb_may_pull(skb,
+ skb_network_offset(skb) + sizeof(new_lse)))
+ goto drop;
new_lse = tcf_mpls_get_lse(mpls_hdr(skb), p, false);
if (skb_mpls_update_lse(skb, new_lse))
goto drop;
--
2.28.0
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH net] net/sched: act_mpls: ensure LSE is pullable before reading it
2020-11-18 16:36 [PATCH net] net/sched: act_mpls: ensure LSE is pullable before reading it Davide Caratti
@ 2020-11-18 16:47 ` Marcelo Ricardo Leitner
2020-11-18 17:07 ` Davide Caratti
2020-11-20 19:24 ` Jakub Kicinski
1 sibling, 1 reply; 5+ messages in thread
From: Marcelo Ricardo Leitner @ 2020-11-18 16:47 UTC (permalink / raw)
To: Davide Caratti
Cc: Jamal Hadi Salim, Cong Wang, Jiri Pirko, netdev, Jakub Kicinski,
gnault
On Wed, Nov 18, 2020 at 05:36:52PM +0100, Davide Caratti wrote:
Hi,
> case TCA_MPLS_ACT_MODIFY:
> + if (!pskb_may_pull(skb,
> + skb_network_offset(skb) + sizeof(new_lse)))
> + goto drop;
> new_lse = tcf_mpls_get_lse(mpls_hdr(skb), p, false);
> if (skb_mpls_update_lse(skb, new_lse))
> goto drop;
Seems TCA_MPLS_ACT_DEC_TTL is also affected. skb_mpls_dec_ttl() will
also call mpls_hdr(skb) without this check.
Marcelo
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net] net/sched: act_mpls: ensure LSE is pullable before reading it
2020-11-18 16:47 ` Marcelo Ricardo Leitner
@ 2020-11-18 17:07 ` Davide Caratti
2020-11-18 18:00 ` Marcelo Ricardo Leitner
0 siblings, 1 reply; 5+ messages in thread
From: Davide Caratti @ 2020-11-18 17:07 UTC (permalink / raw)
To: Marcelo Ricardo Leitner
Cc: Jamal Hadi Salim, Cong Wang, Jiri Pirko, netdev, Jakub Kicinski,
gnault
On Wed, 2020-11-18 at 13:47 -0300, Marcelo Ricardo Leitner wrote:
> On Wed, Nov 18, 2020 at 05:36:52PM +0100, Davide Caratti wrote:
>
> Hi,
>
> > case TCA_MPLS_ACT_MODIFY:
> > + if (!pskb_may_pull(skb,
> > + skb_network_offset(skb) + sizeof(new_lse)))
> > + goto drop;
> > new_lse = tcf_mpls_get_lse(mpls_hdr(skb), p, false);
> > if (skb_mpls_update_lse(skb, new_lse))
> > goto drop;
>
> Seems TCA_MPLS_ACT_DEC_TTL is also affected. skb_mpls_dec_ttl() will
> also call mpls_hdr(skb) without this check.
>
> Marcelo
>
... yes, correct; and at a first glance, also set_mpls() in
openvswitch/action.c has the same (theoretical) issue. I will follow-up
with other 2 patches, ok?
thanks!
--
davide
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net] net/sched: act_mpls: ensure LSE is pullable before reading it
2020-11-18 17:07 ` Davide Caratti
@ 2020-11-18 18:00 ` Marcelo Ricardo Leitner
0 siblings, 0 replies; 5+ messages in thread
From: Marcelo Ricardo Leitner @ 2020-11-18 18:00 UTC (permalink / raw)
To: Davide Caratti
Cc: Jamal Hadi Salim, Cong Wang, Jiri Pirko, netdev, Jakub Kicinski,
gnault
On Wed, Nov 18, 2020 at 06:07:22PM +0100, Davide Caratti wrote:
> On Wed, 2020-11-18 at 13:47 -0300, Marcelo Ricardo Leitner wrote:
> > On Wed, Nov 18, 2020 at 05:36:52PM +0100, Davide Caratti wrote:
> >
> > Hi,
> >
> > > case TCA_MPLS_ACT_MODIFY:
> > > + if (!pskb_may_pull(skb,
> > > + skb_network_offset(skb) + sizeof(new_lse)))
> > > + goto drop;
> > > new_lse = tcf_mpls_get_lse(mpls_hdr(skb), p, false);
> > > if (skb_mpls_update_lse(skb, new_lse))
> > > goto drop;
> >
> > Seems TCA_MPLS_ACT_DEC_TTL is also affected. skb_mpls_dec_ttl() will
> > also call mpls_hdr(skb) without this check.
> >
> > Marcelo
> >
> ... yes, correct; and at a first glance, also set_mpls() in
> openvswitch/action.c has the same (theoretical) issue. I will follow-up
> with other 2 patches, ok?
Yep! Thanks.
Reviewed-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net] net/sched: act_mpls: ensure LSE is pullable before reading it
2020-11-18 16:36 [PATCH net] net/sched: act_mpls: ensure LSE is pullable before reading it Davide Caratti
2020-11-18 16:47 ` Marcelo Ricardo Leitner
@ 2020-11-20 19:24 ` Jakub Kicinski
1 sibling, 0 replies; 5+ messages in thread
From: Jakub Kicinski @ 2020-11-20 19:24 UTC (permalink / raw)
To: Davide Caratti
Cc: Jamal Hadi Salim, Cong Wang, Jiri Pirko, netdev, gnault,
marcelo.leitner
On Wed, 18 Nov 2020 17:36:52 +0100 Davide Caratti wrote:
> when 'act_mpls' is used to mangle the LSE, the current value is read from
> the packet with mpls_hdr(): ensure that the label is contained in the skb
> "linear" area.
>
> Found by code inspection.
>
> Fixes: 2a2ea50870ba ("net: sched: add mpls manipulation actions to TC")
> Signed-off-by: Davide Caratti <dcaratti@redhat.com>
> ---
> net/sched/act_mpls.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/net/sched/act_mpls.c b/net/sched/act_mpls.c
> index 5c7456e5b5cf..03138ad59e9b 100644
> --- a/net/sched/act_mpls.c
> +++ b/net/sched/act_mpls.c
> @@ -105,6 +105,9 @@ static int tcf_mpls_act(struct sk_buff *skb, const struct tc_action *a,
> goto drop;
> break;
> case TCA_MPLS_ACT_MODIFY:
> + if (!pskb_may_pull(skb,
> + skb_network_offset(skb) + sizeof(new_lse)))
nit: MPLS_HLEN?
> + goto drop;
> new_lse = tcf_mpls_get_lse(mpls_hdr(skb), p, false);
> if (skb_mpls_update_lse(skb, new_lse))
> goto drop;
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-11-20 19:24 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-11-18 16:36 [PATCH net] net/sched: act_mpls: ensure LSE is pullable before reading it Davide Caratti
2020-11-18 16:47 ` Marcelo Ricardo Leitner
2020-11-18 17:07 ` Davide Caratti
2020-11-18 18:00 ` Marcelo Ricardo Leitner
2020-11-20 19:24 ` Jakub Kicinski
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).