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

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]

Message-ID: <CADvbK_e_V_scDpHiGw+Qqmarw8huYYES2j8Z36KYkgT2opED3w@mail.gmail.com>
Date:   Sat, 14 Jan 2023 12:52:45 -0500
From:   Xin Long <lucien.xin@...il.com>
To:     Paul Moore <paul@...l-moore.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 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:
> >
> > 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@...il.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