| Message ID | e14a44135817430fc69b3c624895f8584a560975.1605716949.git.dcaratti@redhat.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] net/sched: act_mpls: ensure LSE is pullable before reading it | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net |
| netdev/subject_prefix | success | Link |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 9 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
| netdev/stable | success | Stable not CCed |
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
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!
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>
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;
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;
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(+)