linux-kernel - Re: [PATCH 3/3] creds: convert cred.usage to refcount

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: <b3b09954-9973-8cda-747f-0c43b2de5700@huawei.com>
Date:   Mon, 15 Jun 2020 10:02:55 +0800
From:   Xiaoming Ni <nixiaoming@...wei.com>
To:     Kees Cook <keescook@...omium.org>,
        Peter Zijlstra <peterz@...radead.org>
CC:     Elena Reshetova <elena.reshetova@...el.com>,
        David Windsor <dwindsor@...il.com>,
        Hans Liljestrand <ishkamiel@...il.com>,
        Paul Moore <paul@...l-moore.com>, <edumazet@...gle.com>,
        <paulmck@...nel.org>, "David Howells" <dhowells@...hat.com>,
        <shakeelb@...gle.com>, James Morris <jamorris@...ux.microsoft.com>,
        <alex.huangjianhui@...wei.com>, <dylix.dailei@...wei.com>,
        <chenzefeng2@...wei.com>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 3/3] creds: convert cred.usage to refcount_t

On 2020/6/13 2:34, Kees Cook wrote:
> From: Elena Reshetova <elena.reshetova@...el.com>
> 
> atomic_t variables are currently used to implement reference
> counters with the following properties:
>   - counter is initialized to 1 using atomic_set()
>   - a resource is freed upon counter reaching zero
>   - once counter reaches zero, its further
>     increments aren't allowed
>   - counter schema uses basic atomic operations
>     (set, inc, inc_not_zero, dec_and_test, etc.)
> 
> Such atomic variables should be converted to a newly provided
> refcount_t type and API that prevents accidental counter overflows
> and underflows. This is important since overflows and underflows
> can lead to use-after-free situation and be exploitable.
> 
> The variable cred.usage is used as pure reference counter.
> Convert it to refcount_t and fix up the operations.
> 
> **Important note for maintainers:
> 
> Some functions from refcount_t API defined in lib/refcount.c
> have different memory ordering guarantees than their atomic
> counterparts.Please check Documentation/core-api/refcount-vs-atomic.rst
> for more information.
> 
> Normally the differences should not matter since refcount_t provides
> enough guarantees to satisfy the refcounting use cases, but in
> some rare cases it might matter.
> Please double check that you don't have some undocumented
> memory guarantees for this variable usage.
> 
> For the cred.usage it might make a difference
> in following places:
>   - get_task_cred(): increment in refcount_inc_not_zero() only
>     guarantees control dependency on success vs. fully ordered
>     atomic counterpart
>   - put_cred(): decrement in refcount_dec_and_test() only
>     provides RELEASE ordering and ACQUIRE ordering on success
>     vs. fully ordered atomic counterpart
> 
> Suggested-by: Kees Cook <keescook@...omium.org>
> Signed-off-by: Elena Reshetova <elena.reshetova@...el.com>
> Reviewed-by: David Windsor <dwindsor@...il.com>
> Reviewed-by: Hans Liljestrand <ishkamiel@...il.com>
> Link: https://lore.kernel.org/r/20190306110549.7628-4-elena.reshetova@intel.com
> Signed-off-by: Kees Cook <keescook@...omium.org>

Currently this patch is better than my RFC patch
Looks good to me.

Thanks
Xiaoming Ni