Message-ID: <CAHC9VhR4_ae=QzrUUM=1MZTWJ9MQom0fEAME3b+z+uBrA8PpcQ@mail.gmail.com>
Date:   Tue, 17 Jan 2023 14:51:43 -0500
From:   Paul Moore <paul@...l-moore.com>
To:     Xin Long <lucien.xin@...il.com>
Cc:     network dev <netdev@...r.kernel.org>, davem@...emloft.net,
        kuba@...nel.org, Eric Dumazet <edumazet@...gle.com>,
        Paolo Abeni <pabeni@...hat.com>,
        David Ahern <dsahern@...il.com>,
        Hideaki YOSHIFUJI <yoshfuji@...ux-ipv6.org>,
        Pravin B Shelar <pshelar@....org>,
        Jamal Hadi Salim <jhs@...atatu.com>,
        Cong Wang <xiyou.wangcong@...il.com>,
        Jiri Pirko <jiri@...nulli.us>,
        Pablo Neira Ayuso <pablo@...filter.org>,
        Florian Westphal <fw@...len.de>,
        Marcelo Ricardo Leitner <marcelo.leitner@...il.com>,
        Ilya Maximets <i.maximets@....org>,
        Aaron Conole <aconole@...hat.com>,
        Roopa Prabhu <roopa@...dia.com>,
        Nikolay Aleksandrov <razor@...ckwall.org>,
        Mahesh Bandewar <maheshb@...gle.com>,
        Guillaume Nault <gnault@...hat.com>,
        linux-security-module@...r.kernel.org, selinux@...r.kernel.org
Subject: Re: [PATCH net-next 06/10] cipso_ipv4: use iph_set_totlen in skbuff_setattr

On Mon, Jan 16, 2023 at 2:35 PM Xin Long <lucien.xin@...il.com> wrote:
> On Mon, Jan 16, 2023 at 1:13 PM Paul Moore <paul@...l-moore.com> wrote:
> > On Mon, Jan 16, 2023 at 12:37 PM Xin Long <lucien.xin@...il.com> wrote:
> > > On Mon, Jan 16, 2023 at 11:46 AM Paul Moore <paul@...l-moore.com> wrote:
> > > > On Sat, Jan 14, 2023 at 12:54 PM Xin Long <lucien.xin@...il.com> wrote:
> > > > > On Sat, Jan 14, 2023 at 10:39 AM Paul Moore <paul@...l-moore.com> wrote:
> > > > > > On Fri, Jan 13, 2023 at 10:31 PM Xin Long <lucien.xin@...il.com> wrote:

...

> > > > We can't skip the CIPSO labeling as that would be the network packet
> > > > equivalent of not assigning a owner/group/mode to a file on the
> > > > filesystem, which is a Very Bad Thing :)
> > > >
> > > > I spent a little bit of time this morning looking at the problem and I
> > > > think the right approach is two-fold: first introduce a simple check
> > > > in cipso_v4_skbuff_setattr() which returns -E2BIG if the packet length
> > > > grows beyond 65535.  It's rather crude, but it's a tiny patch and
> > > > should at least ensure that the upper layers (NetLabel and SELinux)
> > > > don't send the packet with a bogus length field; it will result in
> > > > packet drops, but honestly that seems preferable to a mangled packet
> > > > which will likely be dropped at some point in the network anyway.
> > > >
> > > > diff --git a/net/ipv4/cipso_ipv4.c b/net/ipv4/cipso_ipv4.c
> > > > index 6cd3b6c559f0..f19c9beda745 100644
> > > > --- a/net/ipv4/cipso_ipv4.c
> > > > +++ b/net/ipv4/cipso_ipv4.c
> > > > @@ -2183,8 +2183,10 @@ int cipso_v4_skbuff_setattr(struct sk_buff *skb,
> > > >         * that the security label is applied to the packet - we do the same
> > > >         * thing when using the socket options and it hasn't caused a problem,
> > > >         * if we need to we can always revisit this choice later */
> > > > -
> > > >        len_delta = opt_len - opt->optlen;
> > > > +       if ((skb->len + len_delta) > 65535)
> > > > +               return -E2BIG;
> > > > +
> > >
> > > Right, looks crude. :-)
> >
> > Yes, but what else can we do?  There is fragmentation, but that is
> > rather ugly and we would still need a solution for when the don't
> > fragment bit is set.  I'm open to suggestions.
>
> looking at ovs_dp_upcall(), for GSO/GRO packets it goes to
> queue_gso_packets() where it calls __skb_gso_segment()
> to segment it into small segs/skbs, then process these segs instead.
>
> I'm thinking you can try to do the same in cipso_v4_skbuff_setattr(),
> and I don't think 64K non-GSO packets exist in the user environment,
> so taking care of GSO packets should be enough.

Thanks, I'll take a look.

> I just don't know if the security_hook will be able to process these
> smaller segs/skbs after the segment.

As long as the smaller, segmented packets have the IPv4 options
preserved/copied on each smaller packet it should be okay.

> > It seems like there is still ongoing discussion about even enabling
> > BIG TCP for IPv4, however for this discussion let's assume that BIG
> > TCP is merged for IPv4.
> >
> > We really should have a solution that allows CIPSO for both normal and
> > BIG TCP, if we don't we force distros and admins to choose between the
> > two and that isn't good.  We should do better.  If skb->len > 64k in
> > the case of BIG TCP, how is the packet eventually divided/fragmented
> > in such a way that the total length field in the IPv4 header doesn't
> > overflow?  Or is that simply handled at the driver/device layer and we
> > simply set skb->len to whatever the size is, regardless of the 16-bit
>
> Yes, for BIG TCP, 16-bit length is set to 0, and it just uses skb->len
> as the IP packet length.

In the BIG TCP case, when is the IPv4 header zero'd out?  Currently
cipso_v4_skbuff_setattr() is called in the NF_INET_LOCAL_OUT and
NF_INET_FORWARD chains, is there an easy way to distinguish between a
traditional segmentation offload mechanism, e.g. GSO, and BIG TCP?  If
BIG TCP allows for arbitrarily large packets we can just grow the
skb->len value as needed and leave the total length field in the IPv4
header untouched/zero, but we would need to be able to distinguish
between a segmentation offload and BIG TCP.

> > In the GRO case, is it safe to grow the packet such that skb->len is
> > greater than 64k?  I presume that the device/driver is going to split
> > the packet anyway and populate the IPv4 total length fields in the
> > header anyway, right?  If we can't grow the packet beyond 64k, is
> > there some way to signal to the driver/device at runtime that the
> > largest packet we can process is 64k minus 40 bytes (for the IPv4
> > options)?
>
> at runtime, not as far as I know.
> It's a field of the network device that can be modified by:
> # ip link set dev eth0 gro_max_size $MAX_SIZE gso_max_size $MAX_SIZE

I need to look at the OVS case above, but one possibility would be to
have the kernel adjust the GSO size down by 40 bytes when
CONFIG_NETLABEL is enabled, but that isn't a great option, and not
something I consider a first (or second) choice.

-- 
paul-moore.com

Powered by blists - more mailing lists