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