linux-kernel - Re: [PATCH] keys/keyctl: Use kfree

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: <f58c2c8a157714e1860caee9bb005ec469ddb72e.camel@HansenPartnership.com>
Date:   Sat, 23 Jul 2022 10:50:49 -0400
From:   James Bottomley <James.Bottomley@...senPartnership.com>
To:     Siddh Raman Pant <code@...dh.me>,
        Greg KH <gregkh@...uxfoundation.org>
Cc:     David Howells <dhowells@...hat.com>,
        Jarkko Sakkinen <jarkko@...nel.org>,
        James Morris <jmorris@...ei.org>,
        "Serge E. Hallyn" <serge@...lyn.com>,
        linux-security-modules <linux-security-module@...r.kernel.org>,
        keyrings <keyrings@...r.kernel.org>,
        linux-kernel-mentees 
        <linux-kernel-mentees@...ts.linuxfoundation.org>,
        linux-kernel <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] keys/keyctl: Use kfree_rcu instead of kfree

On Sat, 2022-07-23 at 20:05 +0530, Siddh Raman Pant wrote:
> On Sat, 23 Jul 2022 19:35:16 +0530  Greg KH <
> gregkh@...uxfoundation.org> wrote:
> > That does not explain why this change is needed.  What problem does
> > this solve?  Why use RCU if you don't have to?  What functionality
> > did you just change in this commit and why?
> 
> We can avoid a race condition wherein some process tries to access
> them while they are being freed. For instance, the comment on
> `watch_queue_clear()` also states that:
>         /*
>          * Remove all the watches that are contributory to a
> queue.  This has the
>          * potential to race with removal of the watches by the
> destruction of the
>          * objects being watched or with the distribution of
> notifications.
>          */
> And an RCU read critical section is initiated in that function, so we
> should use kfree_rcu() to not unintentionally free it while it is in
> the critical section.

That doesn't apply in this case, does it?  watch and wlist are locally
allocated and neither has been made externally visible if the error leg
is taken, so they should just be locally freed, which is what the code
was doing before this proposed patch.

James