netdev - Re: [PATCH] cipso: subtraction on unsigned hdr

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] [day] [month] [year] [list]

Message-Id: <200903041225.01858.paul.moore@hp.com>
Date:	Wed, 4 Mar 2009 12:25:01 -0500
From:	Paul Moore <paul.moore@...com>
To:	Roel Kluin <roel.kluin@...il.com>,
	"David S. Miller" <davem@...emloft.net>
Cc:	netdev@...r.kernel.org, Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: [PATCH] cipso: subtraction on unsigned hdr_delta

On Tuesday 03 March 2009 05:23:38 pm Roel Kluin wrote:
> hdr_delta is unsigned, so take care not to subtract below 0.
>
> Signed-off-by: Roel Kluin <roel.kluin@...il.com>
> ---
> diff --git a/net/ipv4/cipso_ipv4.c b/net/ipv4/cipso_ipv4.c
> index 7bc9929..b1d862b 100644
> --- a/net/ipv4/cipso_ipv4.c
> +++ b/net/ipv4/cipso_ipv4.c
> @@ -1999,7 +1999,11 @@ void cipso_v4_sock_delattr(struct sock *sk)
>  				iter++;
>  		hdr_delta = opt->optlen;
>  		opt->optlen = (optlen_new + 3) & ~3;
> -		hdr_delta -= opt->optlen;
> +
> +		if (hdr_delta > opt->optlen)
> +			hdr_delta -= opt->optlen;
> +		else
> +			hdr_delta = 0;
>  	} else {
>  		/* only the cipso option was present on the socket so we can
>  		 * remove the entire option struct */

This patch isn't needed since I can't think of any sane reason why the 
hdr_delta subtraction would ever cause an error.  Similarly, I don't think 
David's concerns over the icsk_ext_hdr_len adjustment necessarily apply here 
either.  Let me try and explain, feel free to point out faults in my logic ...

This function, cipso_v4_sock_delattr(), is only used _to_ remove the CIPSO IP 
option from a socket, never to add it.  In the code snippet above we see the 
following three lines:

	hdr_delta = opt->optlen;
	opt->optlen = (optlen_new + 3) & ~3;
	hdr_delta -= opt->optlen;

The first line sets hdr_delta equal to length of the original IP option list 
(length includes the CIPSO option).  The second line calculates the new length 
of the IP option list (without the CIPSO option), complete with padding.  The 
third line subtracts the new option length from hdr_delta which is currently 
equal to original option length.  The only way that hdr_delta could ever be in 
danger of going negative would be if the removal of the CIPSO IP option caused 
the total IP option length to somehow increase; I just can't think how that 
would be possible.

-- 
paul moore
linux @ hp

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html