netdev - Re: [PATCH ipsec, resend 1/1] xfrm: Make set-mark default behavior backward compatible

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 15 Jan 2019 11:05:31 -0800
From:   Benedict Wong <benedictwong@...gle.com>
To:     Eyal Birger <eyal.birger@...il.com>
Cc:     netdev@...r.kernel.org,
        Steffen Klassert <steffen.klassert@...unet.com>,
        tobias@...ongswan.org, Lorenzo Colitti <lorenzo@...gle.com>,
        Nathan Harold <nharold@...gle.com>,
        Maciej Żenczykowski <maze@...gle.com>
Subject: Re: [PATCH ipsec, resend 1/1] xfrm: Make set-mark default behavior
 backward compatible

Hi Eyal,

Thanks for taking a look at this.

> It's a little sad that we didn't distinguish between 'no-mark-provided'
> and 'mark = mask = 0'. IIUC before this fix, explicitly setting mark,
> mask to a) mark = 0, mask = 0xffffffff and b) mark = 0, mask = 0 behaved
> differently, whereas after this fix they will act the same.

Agreed. It's a little hacky, but someone determined to use the entirety
of the mark from the flowi (case b) in the route lookup could use
mark = 0x1, mask = 0x0, and that would give the same results as
mark = 0x0, mask = 0x0. It's a little intuitive though. :)

Perhaps at some future point, it may be worth defining a bit in the
xfrm_state->props->extra_flags as "set-mark provided," At which point
we can separate cases (a) and (b)

Cheers,

Ben.

On Mon, Jan 14, 2019 at 9:11 PM Eyal Birger <eyal.birger@...il.com> wrote:
>
> Hi Benedict,
>
> On Mon, 14 Jan 2019 11:24:38 -0800
> Benedict Wong <benedictwong@...gle.com> wrote:
>
> > Fixes 9b42c1f179a6, which changed the default route lookup behavior
> > for tunnel mode SAs in the outbound direction to use the skb mark,
> > whereas previously mark=0 was used if the output mark was
> > unspecified. In mark-based routing schemes such as Android’s, this
> > change in default behavior causes routing loops or lookup failures.
> >
> > This patch restores the default behavior of using a 0 mark while still
> > incorporating the skb mark if the SET_MARK (and SET_MARK_MASK) is
> > specified.
>
> It's a little sad that we didn't distinguish between 'no-mark-provided'
> and 'mark = mask = 0'. IIUC before this fix, explicitly setting mark,
> mask to a) mark = 0, mask = 0xffffffff and b) mark = 0, mask = 0 behaved
> differently, whereas after this fix they will act the same.
>
> But as it seems there was never support for the (b) scenario and commit
> 9b42c1f179a6 broke the existing behavior, so I'm ok with this fix.
>
> Thanks!
> Eyal.
>
> >
> > Tested with additions to Android's kernel unit test suite:
> > https://android-review.googlesource.com/c/kernel/tests/+/860150
> >
> > Fixes: 9b42c1f179a6 ("xfrm: Extend the output_mark to support input
> > direction and masking") Signed-off-by: Benedict Wong
> > <benedictwong@...gle.com> ---
> >  net/xfrm/xfrm_policy.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
> > index 934492bad8e0..5f574ede1332 100644
> > --- a/net/xfrm/xfrm_policy.c
> > +++ b/net/xfrm/xfrm_policy.c
> > @@ -2600,7 +2600,10 @@ static struct dst_entry
> > *xfrm_bundle_create(struct xfrm_policy *policy,
> > dst_copy_metrics(dst1, dst);
> >               if (xfrm[i]->props.mode != XFRM_MODE_TRANSPORT) {
> > -                     __u32 mark = xfrm_smark_get(fl->flowi_mark,
> > xfrm[i]);
> > +                     __u32 mark = 0;
> > +
> > +                     if (xfrm[i]->props.smark.v ||
> > xfrm[i]->props.smark.m)
> > +                             mark =
> > xfrm_smark_get(fl->flowi_mark, xfrm[i]);
> >                       family = xfrm[i]->props.family;
> >                       dst = xfrm_dst_lookup(xfrm[i], tos,
> > fl->flowi_oif,
>