diff mbox series

[1/2] cipso: fix total option length computation

Message ID 20240416152913.1527166-2-omosnace@redhat.com (mailing list archive)
State Changes Requested
Delegated to: Netdev Maintainers
Headers show
Series cipso: make cipso_v4_skbuff_delattr() fully remove the CIPSO options | expand

Checks

Context Check Description
netdev/series_format warning Target tree name not specified in the subject
netdev/tree_selection success Guessed tree name to be net-next
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 928 this patch: 928
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers fail 1 blamed authors not CCed: jmorris@namei.org; 5 maintainers not CCed: pabeni@redhat.com kuba@kernel.org dsahern@kernel.org jmorris@namei.org edumazet@google.com
netdev/build_clang success Errors and warnings before: 938 this patch: 938
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success Fixes tag looks correct
netdev/build_allmodconfig_warn success Errors and warnings before: 939 this patch: 939
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 26 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 40 this patch: 40
netdev/source_inline success Was 0 now: 0
netdev/contest success net-next-2024-04-16--21-00 (tests: 958)

Commit Message

Ondrej Mosnacek April 16, 2024, 3:29 p.m. UTC
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(-)

Comments

Paul Moore April 16, 2024, 6:39 p.m. UTC | #1
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
Ondrej Mosnacek April 17, 2024, 12:49 p.m. UTC | #2
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.
Paul Moore April 25, 2024, 9:15 p.m. UTC | #3
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 mbox series

Patch

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;