Re: [PATCH net-next 06/10] cipso_ipv4: use iph_set_totlen in skbuff_setattr

Re: [PATCH net-next 06/10] cipso_ipv4: use iph_set_totlen in skbuff_setattr

[Paul Moore]

On Fri, Jan 13, 2023 at 10:31 PM Xin Long <lucien.xin@gmail.com> wrote:
>
> It may process IPv4 TCP GSO packets in cipso_v4_skbuff_setattr(), so
> the iph->tot_len update should use iph_set_totlen().
>
> Note that for these non GSO packets, the new iph tot_len with extra
> iph option len added may become greater than 65535, the old process
> will cast it and set iph->tot_len to it, which is a bug. In theory,
> iph options shouldn't be added for these big packets in here, a fix
> may be needed here in the future. For now this patch is only to set
> iph->tot_len to 0 when it happens.

I'm not entirely clear on the paragraph above, but we do need to be
able to set/modify the IP options in cipso_v4_skbuff_setattr() in
order to support CIPSO labeling.  I'm open to better and/or
alternative solutions compared to what we are doing now, but I can't
support a change that is a bug waiting to bite us.  My apologies if
I'm interpreting your comments incorrectly and that isn't the case
here.

> Signed-off-by: Xin Long <lucien.xin@gmail.com>
> ---
>  net/ipv4/cipso_ipv4.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/ipv4/cipso_ipv4.c b/net/ipv4/cipso_ipv4.c
> index 6cd3b6c559f0..79ae7204e8ed 100644
> --- a/net/ipv4/cipso_ipv4.c
> +++ b/net/ipv4/cipso_ipv4.c
> @@ -2222,7 +2222,7 @@ int cipso_v4_skbuff_setattr(struct sk_buff *skb,
>                 memset((char *)(iph + 1) + buf_len, 0, opt_len - buf_len);
>         if (len_delta != 0) {
>                 iph->ihl = 5 + (opt_len >> 2);
> -               iph->tot_len = htons(skb->len);
> +               iph_set_totlen(iph, skb->len);
>         }
>         ip_send_check(iph);
>
> --
> 2.31.1

--
paul-moore.com

Re: [PATCH net-next 06/10] cipso_ipv4: use iph_set_totlen in skbuff_setattr

[Xin Long]

On Sat, Jan 14, 2023 at 10:39 AM Paul Moore <paul@paul-moore.com> wrote:
>
> On Fri, Jan 13, 2023 at 10:31 PM Xin Long <lucien.xin@gmail.com> wrote:
> >
> > It may process IPv4 TCP GSO packets in cipso_v4_skbuff_setattr(), so
> > the iph->tot_len update should use iph_set_totlen().
> >
> > Note that for these non GSO packets, the new iph tot_len with extra
> > iph option len added may become greater than 65535, the old process
> > will cast it and set iph->tot_len to it, which is a bug. In theory,
> > iph options shouldn't be added for these big packets in here, a fix
> > may be needed here in the future. For now this patch is only to set
> > iph->tot_len to 0 when it happens.
>
> I'm not entirely clear on the paragraph above, but we do need to be
> able to set/modify the IP options in cipso_v4_skbuff_setattr() in
> order to support CIPSO labeling.  I'm open to better and/or
> alternative solutions compared to what we are doing now, but I can't
> support a change that is a bug waiting to bite us.  My apologies if
> I'm interpreting your comments incorrectly and that isn't the case
> here.
setting the IP options may cause the packet size to grow (both iph->tot_len
and skb->len), for example:

before setting it, iph->tot_len=65535,
after setting it, iph->tot_len=65535 + 14 (assume the IP option len is 14)
however, tot_len is 16 bit, and can't be set to "65535 + 14".

Hope the above makes it clearer.

This problem exists with or without this patch. Not sure how it should
be fixed in cipso_v4. I knew tcpmss_tg4() setting TCP options which also
causes the packet size to grow, but it requires no data payload:

        /* There is data after the header so the option can't be added
         * without moving it, and doing so may make the SYN packet
         * itself too large. Accept the packet unmodified instead.
         */
        if (len > tcp_hdrlen)
                return 0;

and then the new iph->tot_len won't exceed 65535.

Not sure if we can skip the big packet, or segment/fragment the big packet
in cipso_v4_skbuff_setattr().

Again, this patch fixes the issue when it's an IPv4 BIG TCP packets, and
it doesn't introduce any new issues or fix any old issues, but only fix
what the IPv4 BIG TCP may introduce.

Thanks.

>
> > Signed-off-by: Xin Long <lucien.xin@gmail.com>
> > ---
> >  net/ipv4/cipso_ipv4.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/net/ipv4/cipso_ipv4.c b/net/ipv4/cipso_ipv4.c
> > index 6cd3b6c559f0..79ae7204e8ed 100644
> > --- a/net/ipv4/cipso_ipv4.c
> > +++ b/net/ipv4/cipso_ipv4.c
> > @@ -2222,7 +2222,7 @@ int cipso_v4_skbuff_setattr(struct sk_buff *skb,
> >                 memset((char *)(iph + 1) + buf_len, 0, opt_len - buf_len);
> >         if (len_delta != 0) {
> >                 iph->ihl = 5 + (opt_len >> 2);
> > -               iph->tot_len = htons(skb->len);
> > +               iph_set_totlen(iph, skb->len);
> >         }
> >         ip_send_check(iph);
> >
> > --
> > 2.31.1
>
> --
> paul-moore.com

Re: [PATCH net-next 06/10] cipso_ipv4: use iph_set_totlen in skbuff_setattr

[Paul Moore]

On Sat, Jan 14, 2023 at 12:54 PM Xin Long <lucien.xin@gmail.com> wrote:
> On Sat, Jan 14, 2023 at 10:39 AM Paul Moore <paul@paul-moore.com> wrote:
> > On Fri, Jan 13, 2023 at 10:31 PM Xin Long <lucien.xin@gmail.com> wrote:
> > >
> > > It may process IPv4 TCP GSO packets in cipso_v4_skbuff_setattr(), so
> > > the iph->tot_len update should use iph_set_totlen().
> > >
> > > Note that for these non GSO packets, the new iph tot_len with extra
> > > iph option len added may become greater than 65535, the old process
> > > will cast it and set iph->tot_len to it, which is a bug. In theory,
> > > iph options shouldn't be added for these big packets in here, a fix
> > > may be needed here in the future. For now this patch is only to set
> > > iph->tot_len to 0 when it happens.
> >
> > I'm not entirely clear on the paragraph above, but we do need to be
> > able to set/modify the IP options in cipso_v4_skbuff_setattr() in
> > order to support CIPSO labeling.  I'm open to better and/or
> > alternative solutions compared to what we are doing now, but I can't
> > support a change that is a bug waiting to bite us.  My apologies if
> > I'm interpreting your comments incorrectly and that isn't the case
> > here.
> setting the IP options may cause the packet size to grow (both iph->tot_len
> and skb->len), for example:
>
> before setting it, iph->tot_len=65535,
> after setting it, iph->tot_len=65535 + 14 (assume the IP option len is 14)
> however, tot_len is 16 bit, and can't be set to "65535 + 14".
>
> Hope the above makes it clearer.

Thanks, it does.

> This problem exists with or without this patch. Not sure how it should
> be fixed in cipso_v4 ...
>
> Not sure if we can skip the big packet, or segment/fragment the big packet
> in cipso_v4_skbuff_setattr().

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;
+
       /* if we don't ensure enough headroom we could panic on the skb_push()
        * call below so make sure we have enough, we are also "mangling" the
        * packet so we should probably do a copy-on-write call anyway */

The second step will be to add a max-length IPv4 option filled with
IPOPT_NOOP to the local sockets in the case of
netlbl_sock_setattr()/NETLBL_NLTYPE_ADDRSELECT.  In this case we would
either end up replacing the padding with a proper CIPSO option or
removing it completely in netlbl_skbuff_setattr(); in either case the
total packet length remains the same or decreases so we should be
"safe".

The forwarded packet case is still a bit of an issue, but I think the
likelihood of someone using 64k max-size IPv4 packets on the wire
*and* passing this traffic through a Linux cross-domain router with
CIPSO (re)labeling is about as close to zero as one can possibly get.
At least given the size check present in step one (patchlet above) the
packet will be safely (?) dropped on the Linux system so an admin will
have some idea where to start looking.

I've got some basic patches written for both, but I need to at least
give them a quick sanity test before posting.

--
paul-moore.com

Re: [PATCH net-next 06/10] cipso_ipv4: use iph_set_totlen in skbuff_setattr

[Xin Long]

On Mon, Jan 16, 2023 at 11:46 AM Paul Moore <paul@paul-moore.com> wrote:
>
> On Sat, Jan 14, 2023 at 12:54 PM Xin Long <lucien.xin@gmail.com> wrote:
> > On Sat, Jan 14, 2023 at 10:39 AM Paul Moore <paul@paul-moore.com> wrote:
> > > On Fri, Jan 13, 2023 at 10:31 PM Xin Long <lucien.xin@gmail.com> wrote:
> > > >
> > > > It may process IPv4 TCP GSO packets in cipso_v4_skbuff_setattr(), so
> > > > the iph->tot_len update should use iph_set_totlen().
> > > >
> > > > Note that for these non GSO packets, the new iph tot_len with extra
> > > > iph option len added may become greater than 65535, the old process
> > > > will cast it and set iph->tot_len to it, which is a bug. In theory,
> > > > iph options shouldn't be added for these big packets in here, a fix
> > > > may be needed here in the future. For now this patch is only to set
> > > > iph->tot_len to 0 when it happens.
> > >
> > > I'm not entirely clear on the paragraph above, but we do need to be
> > > able to set/modify the IP options in cipso_v4_skbuff_setattr() in
> > > order to support CIPSO labeling.  I'm open to better and/or
> > > alternative solutions compared to what we are doing now, but I can't
> > > support a change that is a bug waiting to bite us.  My apologies if
> > > I'm interpreting your comments incorrectly and that isn't the case
> > > here.
> > setting the IP options may cause the packet size to grow (both iph->tot_len
> > and skb->len), for example:
> >
> > before setting it, iph->tot_len=65535,
> > after setting it, iph->tot_len=65535 + 14 (assume the IP option len is 14)
> > however, tot_len is 16 bit, and can't be set to "65535 + 14".
> >
> > Hope the above makes it clearer.
>
> Thanks, it does.
>
> > This problem exists with or without this patch. Not sure how it should
> > be fixed in cipso_v4 ...
> >
> > Not sure if we can skip the big packet, or segment/fragment the big packet
> > in cipso_v4_skbuff_setattr().
>
> 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. :-)
Note that for BIG TCP packets skb->len is bigger than 65535.
(I assume CIPSO labeling will drop BIG TCP packets.)

>        /* if we don't ensure enough headroom we could panic on the skb_push()
>         * call below so make sure we have enough, we are also "mangling" the
>         * packet so we should probably do a copy-on-write call anyway */
>
> The second step will be to add a max-length IPv4 option filled with
> IPOPT_NOOP to the local sockets in the case of
> netlbl_sock_setattr()/NETLBL_NLTYPE_ADDRSELECT.  In this case we would
> either end up replacing the padding with a proper CIPSO option or
> removing it completely in netlbl_skbuff_setattr(); in either case the
> total packet length remains the same or decreases so we should be
> "safe".
sounds better.

>
> The forwarded packet case is still a bit of an issue, but I think the
> likelihood of someone using 64k max-size IPv4 packets on the wire
> *and* passing this traffic through a Linux cross-domain router with
> CIPSO (re)labeling is about as close to zero as one can possibly get.
> At least given the size check present in step one (patchlet above) the
> packet will be safely (?) dropped on the Linux system so an admin will
> have some idea where to start looking.
don't know the likelihood of CIPSO (re)labeling on a Linux cross-domain router.
But 64K packets could be GRO packets merged on the interface (GRO enabled)
of the router, not directly from the wire.

>
> I've got some basic patches written for both, but I need to at least
> give them a quick sanity test before posting.
>
Good that you can take care of it from the security side.

I think a similar problem exists in calipso_skbuff_setattr() in
net/ipv6/calipso.c.

Thanks.

Re: [PATCH net-next 06/10] cipso_ipv4: use iph_set_totlen in skbuff_setattr

[Paul Moore]

On Mon, Jan 16, 2023 at 12:37 PM Xin Long <lucien.xin@gmail.com> wrote:
> On Mon, Jan 16, 2023 at 11:46 AM Paul Moore <paul@paul-moore.com> wrote:
> > On Sat, Jan 14, 2023 at 12:54 PM Xin Long <lucien.xin@gmail.com> wrote:
> > > On Sat, Jan 14, 2023 at 10:39 AM Paul Moore <paul@paul-moore.com> wrote:
> > > > On Fri, Jan 13, 2023 at 10:31 PM Xin Long <lucien.xin@gmail.com> wrote:
> > > > >
> > > > > It may process IPv4 TCP GSO packets in cipso_v4_skbuff_setattr(), so
> > > > > the iph->tot_len update should use iph_set_totlen().
> > > > >
> > > > > Note that for these non GSO packets, the new iph tot_len with extra
> > > > > iph option len added may become greater than 65535, the old process
> > > > > will cast it and set iph->tot_len to it, which is a bug. In theory,
> > > > > iph options shouldn't be added for these big packets in here, a fix
> > > > > may be needed here in the future. For now this patch is only to set
> > > > > iph->tot_len to 0 when it happens.
> > > >
> > > > I'm not entirely clear on the paragraph above, but we do need to be
> > > > able to set/modify the IP options in cipso_v4_skbuff_setattr() in
> > > > order to support CIPSO labeling.  I'm open to better and/or
> > > > alternative solutions compared to what we are doing now, but I can't
> > > > support a change that is a bug waiting to bite us.  My apologies if
> > > > I'm interpreting your comments incorrectly and that isn't the case
> > > > here.
> > > setting the IP options may cause the packet size to grow (both iph->tot_len
> > > and skb->len), for example:
> > >
> > > before setting it, iph->tot_len=65535,
> > > after setting it, iph->tot_len=65535 + 14 (assume the IP option len is 14)
> > > however, tot_len is 16 bit, and can't be set to "65535 + 14".
> > >
> > > Hope the above makes it clearer.
> >
> > Thanks, it does.
> >
> > > This problem exists with or without this patch. Not sure how it should
> > > be fixed in cipso_v4 ...
> > >
> > > Not sure if we can skip the big packet, or segment/fragment the big packet
> > > in cipso_v4_skbuff_setattr().
> >
> > 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.

> Note that for BIG TCP packets skb->len is bigger than 65535.
> (I assume CIPSO labeling will drop BIG TCP packets.)

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
length limit?  If that is the case, does the driver/device layer
handle copying the IPv4 options and setting the header/total-length
fields in each packet?  Or is it something else completely?

> >        /* if we don't ensure enough headroom we could panic on the skb_push()
> >         * call below so make sure we have enough, we are also "mangling" the
> >         * packet so we should probably do a copy-on-write call anyway */
> >
> > The second step will be to add a max-length IPv4 option filled with
> > IPOPT_NOOP to the local sockets in the case of
> > netlbl_sock_setattr()/NETLBL_NLTYPE_ADDRSELECT.  In this case we would
> > either end up replacing the padding with a proper CIPSO option or
> > removing it completely in netlbl_skbuff_setattr(); in either case the
> > total packet length remains the same or decreases so we should be
> > "safe".
>
> sounds better.

To be clear, it's not a one-or-the-other choice, both patches would be
necessary as the padding route described in the second step would only
apply to traffic generated locally from a socket.  We still have the
ugly problem of dealing with forwarded traffic, which it looks like we
discuss more on that below ...

> > The forwarded packet case is still a bit of an issue, but I think the
> > likelihood of someone using 64k max-size IPv4 packets on the wire
> > *and* passing this traffic through a Linux cross-domain router with
> > CIPSO (re)labeling is about as close to zero as one can possibly get.
> > At least given the size check present in step one (patchlet above) the
> > packet will be safely (?) dropped on the Linux system so an admin will
> > have some idea where to start looking.
>
> don't know the likelihood of CIPSO (re)labeling on a Linux cross-domain router.
> But 64K packets could be GRO packets merged on the interface (GRO enabled)
> of the router, not directly from the wire.

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)?

> > I've got some basic patches written for both, but I need to at least
> > give them a quick sanity test before posting.
>
> Good that you can take care of it from the security side.

It's not yet clear to me that we can, see the above questions.

> I think a similar problem exists in calipso_skbuff_setattr() in
> net/ipv6/calipso.c.

Probably, but one mess at a time, especially since several of the
questions above apply to both IPv4 and IPv6.

-- 
paul-moore.com

Re: [PATCH net-next 06/10] cipso_ipv4: use iph_set_totlen in skbuff_setattr

[Xin Long]

On Mon, Jan 16, 2023 at 1:13 PM Paul Moore <paul@paul-moore.com> wrote:
>
> On Mon, Jan 16, 2023 at 12:37 PM Xin Long <lucien.xin@gmail.com> wrote:
> > On Mon, Jan 16, 2023 at 11:46 AM Paul Moore <paul@paul-moore.com> wrote:
> > > On Sat, Jan 14, 2023 at 12:54 PM Xin Long <lucien.xin@gmail.com> wrote:
> > > > On Sat, Jan 14, 2023 at 10:39 AM Paul Moore <paul@paul-moore.com> wrote:
> > > > > On Fri, Jan 13, 2023 at 10:31 PM Xin Long <lucien.xin@gmail.com> wrote:
> > > > > >
> > > > > > It may process IPv4 TCP GSO packets in cipso_v4_skbuff_setattr(), so
> > > > > > the iph->tot_len update should use iph_set_totlen().
> > > > > >
> > > > > > Note that for these non GSO packets, the new iph tot_len with extra
> > > > > > iph option len added may become greater than 65535, the old process
> > > > > > will cast it and set iph->tot_len to it, which is a bug. In theory,
> > > > > > iph options shouldn't be added for these big packets in here, a fix
> > > > > > may be needed here in the future. For now this patch is only to set
> > > > > > iph->tot_len to 0 when it happens.
> > > > >
> > > > > I'm not entirely clear on the paragraph above, but we do need to be
> > > > > able to set/modify the IP options in cipso_v4_skbuff_setattr() in
> > > > > order to support CIPSO labeling.  I'm open to better and/or
> > > > > alternative solutions compared to what we are doing now, but I can't
> > > > > support a change that is a bug waiting to bite us.  My apologies if
> > > > > I'm interpreting your comments incorrectly and that isn't the case
> > > > > here.
> > > > setting the IP options may cause the packet size to grow (both iph->tot_len
> > > > and skb->len), for example:
> > > >
> > > > before setting it, iph->tot_len=65535,
> > > > after setting it, iph->tot_len=65535 + 14 (assume the IP option len is 14)
> > > > however, tot_len is 16 bit, and can't be set to "65535 + 14".
> > > >
> > > > Hope the above makes it clearer.
> > >
> > > Thanks, it does.
> > >
> > > > This problem exists with or without this patch. Not sure how it should
> > > > be fixed in cipso_v4 ...
> > > >
> > > > Not sure if we can skip the big packet, or segment/fragment the big packet
> > > > in cipso_v4_skbuff_setattr().
> > >
> > > 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.

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

>
> > Note that for BIG TCP packets skb->len is bigger than 65535.
> > (I assume CIPSO labeling will drop BIG TCP packets.)
>
> 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.

> length limit?  If that is the case, does the driver/device layer
> handle copying the IPv4 options and setting the header/total-length
> fields in each packet?  Or is it something else completely?
Yes, I think the driver/device layer will handle copying the IPv4 options
and setting the header/total-length, and that's how it works.

>
> > >        /* if we don't ensure enough headroom we could panic on the skb_push()
> > >         * call below so make sure we have enough, we are also "mangling" the
> > >         * packet so we should probably do a copy-on-write call anyway */
> > >
> > > The second step will be to add a max-length IPv4 option filled with
> > > IPOPT_NOOP to the local sockets in the case of
> > > netlbl_sock_setattr()/NETLBL_NLTYPE_ADDRSELECT.  In this case we would
> > > either end up replacing the padding with a proper CIPSO option or
> > > removing it completely in netlbl_skbuff_setattr(); in either case the
> > > total packet length remains the same or decreases so we should be
> > > "safe".
> >
> > sounds better.
>
> To be clear, it's not a one-or-the-other choice, both patches would be
> necessary as the padding route described in the second step would only
> apply to traffic generated locally from a socket.  We still have the
> ugly problem of dealing with forwarded traffic, which it looks like we
> discuss more on that below ...
I got that.

>
> > > The forwarded packet case is still a bit of an issue, but I think the
> > > likelihood of someone using 64k max-size IPv4 packets on the wire
> > > *and* passing this traffic through a Linux cross-domain router with
> > > CIPSO (re)labeling is about as close to zero as one can possibly get.
> > > At least given the size check present in step one (patchlet above) the
> > > packet will be safely (?) dropped on the Linux system so an admin will
> > > have some idea where to start looking.
> >
> > don't know the likelihood of CIPSO (re)labeling on a Linux cross-domain router.
> > But 64K packets could be GRO packets merged on the interface (GRO enabled)
> > of the router, not directly from the wire.
>
> 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've got some basic patches written for both, but I need to at least
> > > give them a quick sanity test before posting.
> >
> > Good that you can take care of it from the security side.
>
> It's not yet clear to me that we can, see the above questions.
>
> > I think a similar problem exists in calipso_skbuff_setattr() in
> > net/ipv6/calipso.c.
>
> Probably, but one mess at a time, especially since several of the
> questions above apply to both IPv4 and IPv6.
>
Just wanted you to know that IPv6 BIG TCP is something already
in the kernel, unlike IPv4 BIG TCP, we can avoid this problem in
advance before it's in the kernel.

Thanks.

Re: [PATCH net-next 06/10] cipso_ipv4: use iph_set_totlen in skbuff_setattr

[David Ahern]

On 1/16/23 12:33 PM, Xin Long wrote:
>> 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.
> 
>> length limit?  If that is the case, does the driver/device layer
>> handle copying the IPv4 options and setting the header/total-length
>> fields in each packet?  Or is it something else completely?
> Yes, I think the driver/device layer will handle copying the IPv4 options
> and setting the header/total-length, and that's how it works.

IPv4 options, like TCP options, should be part of the header that gets
replicate across GSO sliced packets by the hardware. ie., both should be
transparent to well designed hardware (and for h/w that made poor
choices standard 64kB GSO is the limit for its users).

Re: [PATCH net-next 06/10] cipso_ipv4: use iph_set_totlen in skbuff_setattr

[Paul Moore]

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

Re: [PATCH net-next 06/10] cipso_ipv4: use iph_set_totlen in skbuff_setattr

[Paul Moore]

On Tue, Jan 17, 2023 at 2:51 PM Paul Moore <paul@paul-moore.com> wrote:
> On Mon, Jan 16, 2023 at 2:35 PM Xin Long <lucien.xin@gmail.com> wrote:
> > On Mon, Jan 16, 2023 at 1:13 PM Paul Moore <paul@paul-moore.com> wrote:
> > > On Mon, Jan 16, 2023 at 12:37 PM Xin Long <lucien.xin@gmail.com> wrote:
> > > > On Mon, Jan 16, 2023 at 11:46 AM Paul Moore <paul@paul-moore.com> wrote:
> > > > > On Sat, Jan 14, 2023 at 12:54 PM Xin Long <lucien.xin@gmail.com> wrote:
> > > > > > On Sat, Jan 14, 2023 at 10:39 AM Paul Moore <paul@paul-moore.com> wrote:
> > > > > > > On Fri, Jan 13, 2023 at 10:31 PM Xin Long <lucien.xin@gmail.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.

Unfortunately I don't think the ovs_dp_upcall() approach will work as
that is an endpoint in the kernel which sends the GSO'd packet up to
userspace in segements.  In the case of cipso_v4_skbuff_setattr() we
are setting an IPv4 option on a packet in either the NF_INET_LOCAL_OUT
or NF_INET_FORWARD output path.  I believe we can resolve the
LOCAL_OUT case with the padding approach I mentioned previously, but
the FORWARD path remains a challenge; I simply don't see a way to
handle growing the packet beyond 64k in the forward path.  I'm also
realizing that we should be sending a ICMP_FRAG_NEEDED in the forward
case when we have to drop the packet due to size issues, as the normal
MTU/size check happens prior to the NF_INET_FORWARD hooks (and hence
cipso_v4_skbuff_setattr()).

> > > 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.

Keeping the above questions as they still apply, rather I could still
use some help understanding what a BIG TCP packet would look like
during LOCAL_OUT and FORWARD.

> > > 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.

Looking more at the GSO related code, this isn't likely to work.

-- 
paul-moore.com

Re: [PATCH net-next 06/10] cipso_ipv4: use iph_set_totlen in skbuff_setattr

[David Ahern]

On 1/17/23 3:46 PM, Paul Moore wrote:
>>
>> 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.
> 
> Keeping the above questions as they still apply, rather I could still
> use some help understanding what a BIG TCP packet would look like
> during LOCAL_OUT and FORWARD.

skb->len > 64kb. you don't typically look at the IP / IPv6 header and
its total length field and I thought the first patch in the series added
a handler for doing that.

> 
>>>> 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.
> 
> Looking more at the GSO related code, this isn't likely to work.
> 

icsk_ext_hdr_len is adjusted by cipso for its options. Does that not
cover what is needed?

Re: [PATCH net-next 06/10] cipso_ipv4: use iph_set_totlen in skbuff_setattr

[Paul Moore]

On Tue, Jan 17, 2023 at 9:47 PM David Ahern <dsahern@gmail.com> wrote:
> On 1/17/23 3:46 PM, Paul Moore wrote:
> >>
> >> 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.
> >
> > Keeping the above questions as they still apply, rather I could still
> > use some help understanding what a BIG TCP packet would look like
> > during LOCAL_OUT and FORWARD.
>
> skb->len > 64kb. you don't typically look at the IP / IPv6 header and
> its total length field and I thought the first patch in the series added
> a handler for doing that.

Thanks, I was just curious if there was some other mechanism but that works.

As of this moment, the patchset I'm working on is still independent of
the BIG TCP patches, and I want to make sure I'm not doing anything
that will make the BIG TCP patches any more challenging.

> >>>> 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.
> >
> > Looking more at the GSO related code, this isn't likely to work.
>
> icsk_ext_hdr_len is adjusted by cipso for its options. Does that not
> cover what is needed?

Adjusting the icsk_ext_hdr_len only applies to CIPSO labels that are
attached via the associated local sock, traffic that is labeled by
cipso_v4_skbuff_setattr() in the LOCAL_OUT or FORWARD netfilter hooks
does not have the icsk_ext_hdr_len adjustment.

Although as I mentioned earlier, I am adding a patch which would pad
out the IPv4 option header in the LOCAL_OUT labeling scenario so
icsk_ext_hdr_len will be adjusted for all locally generated
TCP/connected/is_icsk traffic.  Forwarded traffic still remains an
issue; but I think the only thing we can do is drop it and send an
icmp message back to the sender with an adjusted MTU value.

--
paul-moore.com