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

[Paul Moore <paul@paul-moore.com>]

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