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] [thread-next>] [day] [month] [year] [list]
Message-ID: <1522358626.2654.39.camel@codethink.co.uk>
Date:   Thu, 29 Mar 2018 22:23:46 +0100
From:   Ben Hutchings <ben.hutchings@...ethink.co.uk>
To:     Gao Feng <fgao@...ai8.com>, "David S. Miller" <davem@...emloft.net>
Cc:     stable@...r.kernel.org,
        Sasha Levin <alexander.levin@...rosoft.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 4.4 033/134] tcp: sysctl: Fix a race to avoid unexpected
 0 window from space

On Mon, 2018-03-19 at 19:05 +0100, Greg Kroah-Hartman wrote:
> 4.4-stable review patch.  If anyone has any objections, please let me know.
> 
> ------------------
> 
> From: Gao Feng <fgao@...ai8.com>
> 
> 
> [ Upstream commit c48367427a39ea0b85c7cf018fe4256627abfd9e ]
> 
> Because sysctl_tcp_adv_win_scale could be changed any time, so there
> is one race in tcp_win_from_space.
> For example,
> 1.sysctl_tcp_adv_win_scale<=0 (sysctl_tcp_adv_win_scale is negative now)
> 2.space>>(-sysctl_tcp_adv_win_scale) (sysctl_tcp_adv_win_scale is postive now)
> 
> As a result, tcp_win_from_space returns 0. It is unexpected.
> 
> Certainly if the compiler put the sysctl_tcp_adv_win_scale into one
> register firstly, then use the register directly, it would be ok.
> But we could not depend on the compiler behavior.

This is true, but the compiler can also decide that this local variable
is just an alias for the global variable and still read it twice.  It
is necessary to use READ_ONCE() to prevent that.

Ben.

> Signed-off-by: Gao Feng <fgao@...ai8.com>
> Signed-off-by: David S. Miller <davem@...emloft.net>
> Signed-off-by: Sasha Levin <alexander.levin@...rosoft.com>
> Signed-off-by: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
> ---
>  include/net/tcp.h |    8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> --- a/include/net/tcp.h
> +++ b/include/net/tcp.h
> @@ -1199,9 +1199,11 @@ void tcp_select_initial_window(int __spa
>  
>  static inline int tcp_win_from_space(int space)
>  {
> -	return sysctl_tcp_adv_win_scale<=0 ?
> -		(space>>(-sysctl_tcp_adv_win_scale)) :
> -		space - (space>>sysctl_tcp_adv_win_scale);
> +	int tcp_adv_win_scale = sysctl_tcp_adv_win_scale;
> +
> +	return tcp_adv_win_scale <= 0 ?
> +		(space>>(-tcp_adv_win_scale)) :
> +		space - (space>>tcp_adv_win_scale);
>  }
>  
>  /* Note: caller must be prepared to deal with negative returns */

-- 
Ben Hutchings
Software Developer, Codethink Ltd.

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ