Christian Brauner <brauner@xxxxxxxxxx> writes:
> On Tue, Sep 24, 2024 at 06:13:39PM GMT, Vinicius Costa Gomes wrote:
>> Vinicius Costa Gomes <vinicius.gomes@xxxxxxxxx> writes:
>>
>> > Miklos Szeredi <miklos@xxxxxxxxxx> writes:
>> >
>> >> On Thu, 22 Aug 2024 at 03:25, Vinicius Costa Gomes
>> >> <vinicius.gomes@xxxxxxxxx> 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
