Re: [PATCH v2 04/16] overlayfs: Document critical override_creds() operations

[Vinicius Costa Gomes <vinicius.gomes@intel.com>]

Christian Brauner <brauner@kernel.org> writes:

> On Tue, Sep 24, 2024 at 06:13:39PM GMT, Vinicius Costa Gomes wrote:
>> Vinicius Costa Gomes <vinicius.gomes@intel.com> writes:
>> 
>> > Miklos Szeredi <miklos@szeredi.hu> writes:
>> >
>> >> On Thu, 22 Aug 2024 at 03:25, Vinicius Costa Gomes
>> >> <vinicius.gomes@intel.com> wrote:
>> >>>
>> >>> Add a comment to these operations that cannot use the _light version
>> >>> of override_creds()/revert_creds(), because during the critical
>> >>> section the struct cred .usage counter might be modified.
>> >>
>> >> Why is it a problem if the usage counter is modified?  Why is the
>> >> counter modified in each of these cases?
>> >>
>> >
>> > Working on getting some logs from the crash that I get when I convert
>> > the remaining cases to use the _light() functions.
>> >
>> 
>> See the log below.
>> 
>> > Perhaps I was wrong on my interpretation of the crash.
>> >
>> 
>> What I am seeing is that ovl_setup_cred_for_create() has a "side
>> effect", it creates another set of credentials, runs the security hooks
>> with this new credentials, and the side effect is that when it returns,
>> by design, 'current->cred' is this new credentials (a third set of
>> credentials).
>
> Well yes, during ovl_setup_cred_for_create() the fs{g,u}id needs to be
> overwritten. But I'm stil confused what the exact problem is as it was
> always clear that ovl_setup_cred_for_create() wouldn't be ported to
> light variants.
>
> /me looks...
>
>> 
>> And this implies that refcounting for this is somewhat tricky, as said
>> in commit d0e13f5bbe4b ("ovl: fix uid/gid when creating over whiteout").
>> 
>> I see two ways forward:
>> 
>> 1. Keep using the non _light() versions in functions that call
>>    ovl_setup_cred_for_create().
>> 2. Change ovl_setup_cred_for_create() so it doesn't drop the "extra"
>>    refcount.
>> 
>> I went with (1), and it still sounds to me like the best way, but I
>> agree that my explanation was not good enough, will add the information
>> I just learned to the commit message and to the code.
>> 
>> Do you see another way forward? Or do you think that I should go with
>> (2)?
>
> ... ok, I understand. Say we have:
>
> ovl_create_tmpfile()
> /* current->cred == ovl->creator_cred without refcount bump /*
> old_cred = ovl_override_creds_light()
> -> ovl_setup_cred_for_create()
>    /* Copy current->cred == ovl->creator_cred */
>    modifiable_cred = prepare_creds()
>
>    /* Override current->cred == modifiable_cred */
>    mounter_creds = override_creds(modifiable_cred)
>
>    /*
>     * And here's the BUG BUG BUG where we decrement the refcount on the
>     * constant mounter_creds.
>     */
>    put_cred(mounter_creds) // BUG BUG BUG
>
>    put_cred(modifiable_creds)
>
> So (1) is definitely the wrong option given that we can get rid of
> refcount decs and incs in the creation path.
>
> Imo, you should do (2) and add a WARN_ON_ONC(). Something like the
> __completely untested__:
>

> diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
> index ab65e98a1def..e246e0172bb6 100644
> --- a/fs/overlayfs/dir.c
> +++ b/fs/overlayfs/dir.c
> @@ -571,7 +571,12 @@ static int ovl_setup_cred_for_create(struct dentry *dentry, struct inode *inode,
>                 put_cred(override_cred);
>                 return err;
>         }
> -       put_cred(override_creds(override_cred));
> +
> +       /*
> +        * We must be called with creator creds already, otherwise we risk
> +        * leaking creds.
> +        */
> +       WARN_ON_ONCE(override_creds(override_cred) != ovl_creds(dentry->d_sb));
>         put_cred(override_cred);
>
>         return 0;
>

At first glance, looks good. Going to test it and see how it works.
Thank you.

For the next version of the series, my plan is to include this
suggestion/change and remove the guard()/scoped_guard() conversion
patches from the series.


Cheers,
-- 
Vinicius