linux-kernel - Re: [RFC PATCH 07/11] drivers/android/binder: convert stats, transaction_log to counter

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: <202009231350.8343298C23@keescook>
Date:   Wed, 23 Sep 2020 13:51:29 -0700
From:   Kees Cook <keescook@...omium.org>
To:     Greg KH <gregkh@...uxfoundation.org>
Cc:     devel@...verdev.osuosl.org, tkjos@...roid.com, surenb@...gle.com,
        linux-kernel@...r.kernel.org, hridya@...gle.com, arve@...roid.com,
        Shuah Khan <skhan@...uxfoundation.org>, joel@...lfernandes.org,
        maco@...roid.com, christian@...uner.io
Subject: Re: [RFC PATCH 07/11] drivers/android/binder: convert stats,
 transaction_log to counter_atomic

On Wed, Sep 23, 2020 at 09:31:34PM +0200, Greg KH wrote:
> On Wed, Sep 23, 2020 at 12:04:58PM -0700, Kees Cook wrote:
> > On Wed, Sep 23, 2020 at 07:10:27AM +0200, Greg KH wrote:
> > > On Tue, Sep 22, 2020 at 07:43:36PM -0600, Shuah Khan wrote:
> > > >  struct binder_stats {
> > > > -	atomic_t br[_IOC_NR(BR_FAILED_REPLY) + 1];
> > > > -	atomic_t bc[_IOC_NR(BC_REPLY_SG) + 1];
> > > > -	atomic_t obj_created[BINDER_STAT_COUNT];
> > > > -	atomic_t obj_deleted[BINDER_STAT_COUNT];
> > > > +	struct counter_atomic br[_IOC_NR(BR_FAILED_REPLY) + 1];
> > > > +	struct counter_atomic bc[_IOC_NR(BC_REPLY_SG) + 1];
> > > > +	struct counter_atomic obj_created[BINDER_STAT_COUNT];
> > > > +	struct counter_atomic obj_deleted[BINDER_STAT_COUNT];
> > > 
> > > These are just debugging statistics, no reason they have to be atomic
> > > variables at all and they should be able to just be "struct counter"
> > > variables instead.
> > 
> > But there's no reason for them _not_ to be atomic. Please let's keep
> > this API as always safe. Why even provide a new foot-gun here?
> 
> These are debugging things, how can you shoot yourself in the foot with
> that???

Because suddenly you might be trying to use these values for debugging
only to dig and dig to discover that because they were non-atomic, some
parallel race cause a counter to get dropped, etc.

Since we can design this API robustly, let's take the opportunity to do
so.

-- 
Kees Cook