linux-kernel - Re: [RFC][PATCH 00/15] KEYS: Fixes

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: <20171017175206.GB555@zzz.localdomain>
Date:   Tue, 17 Oct 2017 10:52:06 -0700
From:   Eric Biggers <ebiggers3@...il.com>
To:     David Howells <dhowells@...hat.com>
Cc:     ebiggers@...gle.com, linux-security-module@...r.kernel.org,
        keyrings@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [RFC][PATCH 00/15] KEYS: Fixes

On Mon, Oct 16, 2017 at 11:27:22PM +0100, David Howells wrote:
> Okay, I've fixed those issues, I think.  I've renamed the instantiation labels
> to positive.
> 
> Thanks,
> David
> ---
> commit f23f3bb0ba3be44e775ac74148157136b919e3b0
> Author: David Howells <dhowells@...hat.com>
> Date:   Wed Oct 4 16:43:25 2017 +0100
> 
>     KEYS: Fix race between updating and finding a negative key
>     
>     Consolidate KEY_FLAG_INSTANTIATED, KEY_FLAG_NEGATIVE and the rejection
>     error into one field such that:
>     
>      (1) The instantiation state can be modified/read atomically.
>     
>      (2) The error can be accessed atomically with the state.
>     
>      (3) The error isn't stored unioned with the payload pointers.
>     
>     This deals with the problem that the state is spread over three different
>     objects (two bits and a separate variable) and reading or updating them
>     atomically isn't practical, given that not only can uninstantiated keys
>     change into instantiated or rejected keys, but rejected keys can also turn
>     into instantiated keys - and someone accessing the key might not be using
>     any locking.
>     
>     The main side effect of this problem is that what was held in the payload
>     may change, depending on the state.  For instance, you might observe the
>     key to be in the rejected state.  You then read the cached error, but if
>     the key semaphore wasn't locked, the key might've become instantiated
>     between the two reads - and you might now have something in hand that isn't
>     actually an error code.
>     
>     The state is now KEY_IS_UNINSTANTIATED, KEY_IS_POSITIVE or a negative error
>     code if the key is negatively instantiated.  The key_is_instantiated()
>     function is replaced with key_is_positive() to avoid confusion as negative
>     keys are also 'instantiated'.
>     
>     Additionally, barriering is included:
>     
>      (1) Order payload-set before state-set during instantiation.
>     
>      (2) Order state-read before payload-read when using the key.
>     
>     Further separate barriering is necessary if RCU is being used to access the
>     payload content after reading the payload pointers.
>     
>     Fixes: 146aa8b1453b ("KEYS: Merge the type-specific data with the payload data")
>     Cc: stable@...r.kernel.org # v4.4+
>     Reported-by: Eric Biggers <ebiggers@...gle.com>
>     Signed-off-by: David Howells <dhowells@...hat.com>
> 

This looks good now; feel free to add

	Reviewed-by: Eric Biggers <ebiggers@...gle.com>