| Message ID | 20240416152913.1527166-2-omosnace@redhat.com (mailing list archive) |
|---|---|
| State | Under Review |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | cipso: make cipso_v4_skbuff_delattr() fully remove the CIPSO options | expand |
On Apr 16, 2024 Ondrej Mosnacek <omosnace@redhat.com> wrote: > > As evident from the definition of ip_options_get(), the IP option > IPOPT_END is used to pad the IP option data array, not IPOPT_NOP. Yet > the loop that walks the IP options to determine the total IP options > length in cipso_v4_delopt() doesn't take it into account. > > Fix it by recognizing the IPOPT_END value as the end of actual options. > Also add safety checks in case the options are invalid/corrupted. > > Fixes: 014ab19a69c3 ("selinux: Set socket NetLabel based on connection endpoint") > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com> > --- > net/ipv4/cipso_ipv4.c | 19 ++++++++++++++----- > 1 file changed, 14 insertions(+), 5 deletions(-) > > diff --git a/net/ipv4/cipso_ipv4.c b/net/ipv4/cipso_ipv4.c > index 8b17d83e5fde4..75b5e3c35f9bf 100644 > --- a/net/ipv4/cipso_ipv4.c > +++ b/net/ipv4/cipso_ipv4.c > @@ -2012,12 +2012,21 @@ static int cipso_v4_delopt(struct ip_options_rcu __rcu **opt_ptr) > * from there we can determine the new total option length */ > iter = 0; > optlen_new = 0; > - while (iter < opt->opt.optlen) > - if (opt->opt.__data[iter] != IPOPT_NOP) { > - iter += opt->opt.__data[iter + 1]; > - optlen_new = iter; > - } else > + while (iter < opt->opt.optlen) { > + if (opt->opt.__data[iter] == IPOPT_END) { > + break; > + } else if (opt->opt.__data[iter] == IPOPT_NOP) { > iter++; > + } else { > + if (WARN_ON(opt->opt.__data[iter + 1] < 2)) > + iter += 2; > + else > + iter += opt->opt.__data[iter + 1]; > + optlen_new = iter; I worry that WARN_ON(), especially in conjunction with the one below, could generate a lot of noise on the console and system logs, let's be a bit more selective about what we check and report on. Presumably the options have already gone through a basic sanity check so there shouldn't be anything too scary in there. if (opt == IPOPT_END) { /* ... */ } else if (opt == IPOPT_NOP) { /* ... */ } else { iter += opt[iter + 1]; optlen_new = iter; } > + } > + } > + if (WARN_ON(optlen_new > opt->opt.optlen)) > + optlen_new = opt->opt.optlen; This is also probably not really necessary, but it bothers me less. > hdr_delta = opt->opt.optlen; > opt->opt.optlen = (optlen_new + 3) & ~3; > hdr_delta -= opt->opt.optlen; > -- > 2.44.0 -- paul-moore.com
On Tue, Apr 16, 2024 at 8:39 PM Paul Moore <paul@paul-moore.com> wrote: > > On Apr 16, 2024 Ondrej Mosnacek <omosnace@redhat.com> wrote: > > > > As evident from the definition of ip_options_get(), the IP option > > IPOPT_END is used to pad the IP option data array, not IPOPT_NOP. Yet > > the loop that walks the IP options to determine the total IP options > > length in cipso_v4_delopt() doesn't take it into account. > > > > Fix it by recognizing the IPOPT_END value as the end of actual options. > > Also add safety checks in case the options are invalid/corrupted. > > > > Fixes: 014ab19a69c3 ("selinux: Set socket NetLabel based on connection endpoint") > > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com> > > --- > > net/ipv4/cipso_ipv4.c | 19 ++++++++++++++----- > > 1 file changed, 14 insertions(+), 5 deletions(-) > > > > diff --git a/net/ipv4/cipso_ipv4.c b/net/ipv4/cipso_ipv4.c > > index 8b17d83e5fde4..75b5e3c35f9bf 100644 > > --- a/net/ipv4/cipso_ipv4.c > > +++ b/net/ipv4/cipso_ipv4.c > > @@ -2012,12 +2012,21 @@ static int cipso_v4_delopt(struct ip_options_rcu __rcu **opt_ptr) > > * from there we can determine the new total option length */ > > iter = 0; > > optlen_new = 0; > > - while (iter < opt->opt.optlen) > > - if (opt->opt.__data[iter] != IPOPT_NOP) { > > - iter += opt->opt.__data[iter + 1]; > > - optlen_new = iter; > > - } else > > + while (iter < opt->opt.optlen) { > > + if (opt->opt.__data[iter] == IPOPT_END) { > > + break; > > + } else if (opt->opt.__data[iter] == IPOPT_NOP) { > > iter++; > > + } else { > > + if (WARN_ON(opt->opt.__data[iter + 1] < 2)) > > + iter += 2; > > + else > > + iter += opt->opt.__data[iter + 1]; > > + optlen_new = iter; > > I worry that WARN_ON(), especially in conjunction with the one below, > could generate a lot of noise on the console and system logs, let's > be a bit more selective about what we check and report on. Presumably > the options have already gone through a basic sanity check so there > shouldn't be anything too scary in there. > > if (opt == IPOPT_END) { > /* ... */ > } else if (opt == IPOPT_NOP) { > /* ... */ > } else { > iter += opt[iter + 1]; > optlen_new = iter; > } How about turning it to WARN_ON_ONCE() instead? It's actually the better choice in this case (alerts to a possible kernel bug, not to an event that would need to be logged every time), I just used WARN_ON() instinctively and didn't think of the _ONCE variant. > > > + } > > + } > > + if (WARN_ON(optlen_new > opt->opt.optlen)) > > + optlen_new = opt->opt.optlen; > > This is also probably not really necessary, but it bothers me less. I would convert this one to WARN_ON_ONCE() as well, or drop both if you still don't like either of them to be there. > > > hdr_delta = opt->opt.optlen; > > opt->opt.optlen = (optlen_new + 3) & ~3; > > hdr_delta -= opt->opt.optlen; > > -- > > 2.44.0 > > -- > paul-moore.com > -- Ondrej Mosnacek Senior Software Engineer, Linux Security - SELinux kernel Red Hat, Inc.
On Wed, Apr 17, 2024 at 8:49 AM Ondrej Mosnacek <omosnace@redhat.com> wrote: > On Tue, Apr 16, 2024 at 8:39 PM Paul Moore <paul@paul-moore.com> wrote: > > On Apr 16, 2024 Ondrej Mosnacek <omosnace@redhat.com> wrote: > > > > > > As evident from the definition of ip_options_get(), the IP option > > > IPOPT_END is used to pad the IP option data array, not IPOPT_NOP. Yet > > > the loop that walks the IP options to determine the total IP options > > > length in cipso_v4_delopt() doesn't take it into account. > > > > > > Fix it by recognizing the IPOPT_END value as the end of actual options. > > > Also add safety checks in case the options are invalid/corrupted. > > > > > > Fixes: 014ab19a69c3 ("selinux: Set socket NetLabel based on connection endpoint") > > > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com> > > > --- > > > net/ipv4/cipso_ipv4.c | 19 ++++++++++++++----- > > > 1 file changed, 14 insertions(+), 5 deletions(-) > > > > > > diff --git a/net/ipv4/cipso_ipv4.c b/net/ipv4/cipso_ipv4.c > > > index 8b17d83e5fde4..75b5e3c35f9bf 100644 > > > --- a/net/ipv4/cipso_ipv4.c > > > +++ b/net/ipv4/cipso_ipv4.c > > > @@ -2012,12 +2012,21 @@ static int cipso_v4_delopt(struct ip_options_rcu __rcu **opt_ptr) > > > * from there we can determine the new total option length */ > > > iter = 0; > > > optlen_new = 0; > > > - while (iter < opt->opt.optlen) > > > - if (opt->opt.__data[iter] != IPOPT_NOP) { > > > - iter += opt->opt.__data[iter + 1]; > > > - optlen_new = iter; > > > - } else > > > + while (iter < opt->opt.optlen) { > > > + if (opt->opt.__data[iter] == IPOPT_END) { > > > + break; > > > + } else if (opt->opt.__data[iter] == IPOPT_NOP) { > > > iter++; > > > + } else { > > > + if (WARN_ON(opt->opt.__data[iter + 1] < 2)) > > > + iter += 2; > > > + else > > > + iter += opt->opt.__data[iter + 1]; > > > + optlen_new = iter; > > > > I worry that WARN_ON(), especially in conjunction with the one below, > > could generate a lot of noise on the console and system logs, let's > > be a bit more selective about what we check and report on. Presumably > > the options have already gone through a basic sanity check so there > > shouldn't be anything too scary in there. > > > > if (opt == IPOPT_END) { > > /* ... */ > > } else if (opt == IPOPT_NOP) { > > /* ... */ > > } else { > > iter += opt[iter + 1]; > > optlen_new = iter; > > } > > How about turning it to WARN_ON_ONCE() instead? It's actually the > better choice in this case (alerts to a possible kernel bug, not to an > event that would need to be logged every time), I just used WARN_ON() > instinctively and didn't think of the _ONCE variant. I'd really prefer to not have the WARN_ON(), even the _ONCE() variant. We're seeing more and more discussion about avoiding the use of WARN_ON() similar to the current BUG_ON() guidelines. > > > + } > > > + } > > > + if (WARN_ON(optlen_new > opt->opt.optlen)) > > > + optlen_new = opt->opt.optlen; > > > > This is also probably not really necessary, but it bothers me less. > > I would convert this one to WARN_ON_ONCE() as well, or drop both if > you still don't like either of them to be there. My preference would be to drop both, although as I said earlier this last one bothers me less.
diff --git a/net/ipv4/cipso_ipv4.c b/net/ipv4/cipso_ipv4.c index 8b17d83e5fde4..75b5e3c35f9bf 100644 --- a/net/ipv4/cipso_ipv4.c +++ b/net/ipv4/cipso_ipv4.c @@ -2012,12 +2012,21 @@ static int cipso_v4_delopt(struct ip_options_rcu __rcu **opt_ptr) * from there we can determine the new total option length */ iter = 0; optlen_new = 0; - while (iter < opt->opt.optlen) - if (opt->opt.__data[iter] != IPOPT_NOP) { - iter += opt->opt.__data[iter + 1]; - optlen_new = iter; - } else + while (iter < opt->opt.optlen) { + if (opt->opt.__data[iter] == IPOPT_END) { + break; + } else if (opt->opt.__data[iter] == IPOPT_NOP) { iter++; + } else { + if (WARN_ON(opt->opt.__data[iter + 1] < 2)) + iter += 2; + else + iter += opt->opt.__data[iter + 1]; + optlen_new = iter; + } + } + if (WARN_ON(optlen_new > opt->opt.optlen)) + optlen_new = opt->opt.optlen; hdr_delta = opt->opt.optlen; opt->opt.optlen = (optlen_new + 3) & ~3; hdr_delta -= opt->opt.optlen;
As evident from the definition of ip_options_get(), the IP option IPOPT_END is used to pad the IP option data array, not IPOPT_NOP. Yet the loop that walks the IP options to determine the total IP options length in cipso_v4_delopt() doesn't take it into account. Fix it by recognizing the IPOPT_END value as the end of actual options. Also add safety checks in case the options are invalid/corrupted. Fixes: 014ab19a69c3 ("selinux: Set socket NetLabel based on connection endpoint") Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com> --- net/ipv4/cipso_ipv4.c | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-)