netdev - Re: [PATCH v2] net: ipv4: emulate READ_ONCE() on ->hdrincl bit-field in raw

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: <20180108161140.796eb4cf@elisabeth>
Date:   Mon, 8 Jan 2018 16:11:40 +0100
From:   Stefano Brivio <sbrivio@...hat.com>
To:     Nicolai Stange <nstange@...e.de>
Cc:     "David S. Miller" <davem@...emloft.net>,
        Alexey Kuznetsov <kuznet@....inr.ac.ru>,
        Hideaki YOSHIFUJI <yoshfuji@...ux-ipv6.org>,
        Mohamed Ghannam <simo.ghannam@...il.com>,
        Michal Kubecek <mkubecek@...e.cz>,
        Miroslav Benes <mbenes@...e.cz>, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] net: ipv4: emulate READ_ONCE() on ->hdrincl
 bit-field in raw_sendmsg()

On Mon,  8 Jan 2018 15:54:44 +0100
Nicolai Stange <nstange@...e.de> wrote:

> Commit 8f659a03a0ba ("net: ipv4: fix for a race condition in
> raw_sendmsg") fixed the issue of possibly inconsistent ->hdrincl handling
> due to concurrent updates by reading this bit-field member into a local
> variable and using the thus stabilized value in subsequent tests.
> 
> However, aforementioned commit also adds the (correct) comment that
> 
>   /* hdrincl should be READ_ONCE(inet->hdrincl)
>    * but READ_ONCE() doesn't work with bit fields
>    */
> 
> because as it stands, the compiler is free to shortcut or even eliminate
> the local variable at its will.
> 
> Note that I have not seen anything like this happening in reality and thus,
> the concern is a theoretical one.
> 
> However, in order to be on the safe side, emulate a READ_ONCE() on the
> bit-field by doing it on the local 'hdrincl' variable itself:
> 
> 	int hdrincl = inet->hdrincl;
> 	hdrincl = READ_ONCE(hdrincl);
> 
> This breaks the chain in the sense that the compiler is not allowed
> to replace subsequent reads from hdrincl with reloads from inet->hdrincl.
> 
> Fixes: 8f659a03a0ba ("net: ipv4: fix for a race condition in raw_sendmsg")
> Signed-off-by: Nicolai Stange <nstange@...e.de>

Reviewed-by: Stefano Brivio <sbrivio@...hat.com>

-- 
Stefano