[PATCH] Smack: Restore the smackfsdef mount option

[PATCH] Smack: Restore the smackfsdef mount option

[Casey Schaufler]

The 5.1 mount system rework changed the smackfsdef mount option
to smackfsdefault. This fixes the regression by making smackfsdef
treated the same way as smackfsdefault. The change was made in
commit c3300aaf95fb4 from Al Viro.

Reported-by: Jose Bollo <jose.bollo@iot.bzh>
Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
---
  security/smack/smack_lsm.c | 2 ++
  1 file changed, 2 insertions(+)

diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index b9abcdb36a73..915cf598e164 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -68,6 +68,7 @@ static struct {
  	int len;
  	int opt;
  } smk_mount_opts[] = {
+	{"smackfsdef", sizeof("smackfsdef") - 1, Opt_fsdefault},
  	A(fsdefault), A(fsfloor), A(fshat), A(fsroot), A(fstransmute)
  };
  #undef A
@@ -682,6 +683,7 @@ static int smack_fs_context_dup(struct fs_context *fc,
  }
  
  static const struct fs_parameter_spec smack_param_specs[] = {
+	fsparam_string("fsdef",		Opt_fsdefault),
  	fsparam_string("fsdefault",	Opt_fsdefault),
  	fsparam_string("fsfloor",	Opt_fsfloor),
  	fsparam_string("fshat",		Opt_fshat),

Re: [PATCH] Smack: Restore the smackfsdef mount option

[Casey Schaufler]

On 5/20/2019 3:48 PM, Casey Schaufler wrote:
> The 5.1 mount system rework changed the smackfsdef mount option
> to smackfsdefault. This fixes the regression by making smackfsdef
> treated the same way as smackfsdefault. The change was made in
> commit c3300aaf95fb4 from Al Viro.
>
> Reported-by: Jose Bollo <jose.bollo@iot.bzh>
> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>

Al, Dave, is this patch in keeping with the intent
of the mount rework? Is there a different way I should
do it? Do you want to take it as a fix for the mount
work, or should I push it?

Thank you.

> ---
> ??security/smack/smack_lsm.c | 2 ++
> ??1 file changed, 2 insertions(+)
>
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index b9abcdb36a73..915cf598e164 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -68,6 +68,7 @@ static struct {
> ???????? int len;
> ???????? int opt;
> ??} smk_mount_opts[] = {
> +?????? {"smackfsdef", sizeof("smackfsdef") - 1, Opt_fsdefault},
> ???????? A(fsdefault), A(fsfloor), A(fshat), A(fsroot), A(fstransmute)
> ??};
> ??#undef A
> @@ -682,6 +683,7 @@ static int smack_fs_context_dup(struct fs_context 
> *fc,
> ??}
>
> ??static const struct fs_parameter_spec smack_param_specs[] = {
> +?????? fsparam_string("fsdef",?????????????? Opt_fsdefault),
> ???????? fsparam_string("fsdefault",?????? Opt_fsdefault),
> ???????? fsparam_string("fsfloor",?????? Opt_fsfloor),
> ???????? fsparam_string("fshat",?????????????? Opt_fshat),
>

Re: [PATCH] Smack: Restore the smackfsdef mount option

[David Howells]

Casey Schaufler <casey@schaufler-ca.com> wrote:

> The change was made in commit c3300aaf95fb4 from Al Viro.

This should be in a "Fixes:" tag?

> +	fsparam_string("fsdef",		Opt_fsdefault),
>  	fsparam_string("fsdefault",	Opt_fsdefault),
>  	fsparam_string("fsfloor",	Opt_fsfloor),
>  	fsparam_string("fshat",		Opt_fshat),

Would it be better to delete the "fsdefault" line?

Also, should all of these be prefixed with "smack"?  So:

  	fsparam_string("smackfsdef",	Opt_fsdefault),
  	fsparam_string("smackfsfloor",	Opt_fsfloor),
  	fsparam_string("smackfshat",	Opt_fshat),	

David

Re: [PATCH] Smack: Restore the smackfsdef mount option

[Casey Schaufler]

On 5/28/2019 5:23 AM, David Howells wrote:
> Casey Schaufler <casey@schaufler-ca.com> wrote:
>
>> The change was made in commit c3300aaf95fb4 from Al Viro.
> This should be in a "Fixes:" tag?

Thanks. I wasn't sure how to properly apply that.

>
>> +	fsparam_string("fsdef",		Opt_fsdefault),
>>  	fsparam_string("fsdefault",	Opt_fsdefault),
>>  	fsparam_string("fsfloor",	Opt_fsfloor),
>>  	fsparam_string("fshat",		Opt_fshat),
> Would it be better to delete the "fsdefault" line?

If it hadn't slipped into the 5.1 release I would
say to remove it, but now it would be a regression.

>
> Also, should all of these be prefixed with "smack"?  So:
>
>   	fsparam_string("smackfsdef",	Opt_fsdefault),
>   	fsparam_string("smackfsfloor",	Opt_fsfloor),
>   	fsparam_string("smackfshat",	Opt_fshat),	

No. smack_fs_parameters takes care of that.

>
> David

Re: [PATCH] Smack: Restore the smackfsdef mount option

[David Howells]

Casey Schaufler <casey@schaufler-ca.com> wrote:

> > Also, should all of these be prefixed with "smack"?  So:
> >
> >   	fsparam_string("smackfsdef",	Opt_fsdefault),
> >   	fsparam_string("smackfsfloor",	Opt_fsfloor),
> >   	fsparam_string("smackfshat",	Opt_fshat),	
> 
> No. smack_fs_parameters takes care of that.

It does?  *Blink*.

smack_fs_parameters.name is just for decorating messages, if that's what
you're looking at.

David

Re: [PATCH] Smack: Restore the smackfsdef mount option

[Casey Schaufler]

On 5/28/2019 9:22 AM, David Howells wrote:
> Casey Schaufler <casey@schaufler-ca.com> wrote:
>
>>> Also, should all of these be prefixed with "smack"?  So:
>>>
>>>   	fsparam_string("smackfsdef",	Opt_fsdefault),
>>>   	fsparam_string("smackfsfloor",	Opt_fsfloor),
>>>   	fsparam_string("smackfshat",	Opt_fshat),	
>> No. smack_fs_parameters takes care of that.
> It does?  *Blink*.

Well, something does. I can't say that I 100% understand all
of how the new mount code handles the mount options. Y'all made
sweeping changes, and the code works the way it used to except
for the awkward change from smackfsdef to smackfsdefault. It
took no small amount of head scratching and experimentation to
convince myself that the fix I proposed was correct.

>
> smack_fs_parameters.name is just for decorating messages, if that's what
> you're looking at.
>
> David

Re: [PATCH] Smack: Restore the smackfsdef mount option

[David Howells]

Casey Schaufler <casey@schaufler-ca.com> wrote:

> > Casey Schaufler <casey@schaufler-ca.com> wrote:
> >
> >>> Also, should all of these be prefixed with "smack"?  So:
> >>>
> >>>   	fsparam_string("smackfsdef",	Opt_fsdefault),
> >>>   	fsparam_string("smackfsfloor",	Opt_fsfloor),
> >>>   	fsparam_string("smackfshat",	Opt_fshat),	
> >> No. smack_fs_parameters takes care of that.
> > It does?  *Blink*.
> 
> Well, something does. I can't say that I 100% understand all
> of how the new mount code handles the mount options. Y'all made
> sweeping changes, and the code works the way it used to except
> for the awkward change from smackfsdef to smackfsdefault. It
> took no small amount of head scratching and experimentation to
> convince myself that the fix I proposed was correct.

Ah...  I suspect the issue is that smack_sb_eat_lsm_opts() strips the prefix
for an unconverted filesystem, but smack_fs_context_parse_param() doesn't
(which it shouldn't).

Can you try grabbing my mount-api-viro branch from:

	https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git

and testing setting smack options on a tmpfs filesystem?

You might need to try modifying samples/vfs/test-fsmount.c to make it mount a
trmpfs filesystem through the new mount UAPI.

David

Re: [PATCH] Smack: Restore the smackfsdef mount option

[Casey Schaufler]

On 5/28/2019 11:54 AM, David Howells wrote:
> Casey Schaufler <casey@schaufler-ca.com> wrote:
>
>>> Casey Schaufler <casey@schaufler-ca.com> wrote:
>>>
>>>>> Also, should all of these be prefixed with "smack"?  So:
>>>>>
>>>>>   	fsparam_string("smackfsdef",	Opt_fsdefault),
>>>>>   	fsparam_string("smackfsfloor",	Opt_fsfloor),
>>>>>   	fsparam_string("smackfshat",	Opt_fshat),	
>>>> No. smack_fs_parameters takes care of that.
>>> It does?  *Blink*.
>> Well, something does. I can't say that I 100% understand all
>> of how the new mount code handles the mount options. Y'all made
>> sweeping changes, and the code works the way it used to except
>> for the awkward change from smackfsdef to smackfsdefault. It
>> took no small amount of head scratching and experimentation to
>> convince myself that the fix I proposed was correct.
> Ah...  I suspect the issue is that smack_sb_eat_lsm_opts() strips the prefix
> for an unconverted filesystem, but smack_fs_context_parse_param() doesn't
> (which it shouldn't).
>
> Can you try grabbing my mount-api-viro branch from:
>
> 	https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git
>
> and testing setting smack options on a tmpfs filesystem?

My fedora system won't boot because smackfsdef isn't recognized. :(
I will put in my fix and retry.

>
> You might need to try modifying samples/vfs/test-fsmount.c to make it mount a
> trmpfs filesystem through the new mount UAPI.
>
> David

Re: [PATCH] Smack: Restore the smackfsdef mount option

[Casey Schaufler]

On 5/28/2019 12:57 PM, Casey Schaufler wrote:
> On 5/28/2019 11:54 AM, David Howells wrote:
>> Casey Schaufler <casey@schaufler-ca.com> wrote:
>>
>>>> Casey Schaufler <casey@schaufler-ca.com> wrote:
>>>>
>>>>>> Also, should all of these be prefixed with "smack"?  So:
>>>>>>
>>>>>>   	fsparam_string("smackfsdef",	Opt_fsdefault),
>>>>>>   	fsparam_string("smackfsfloor",	Opt_fsfloor),
>>>>>>   	fsparam_string("smackfshat",	Opt_fshat),	
>>>>> No. smack_fs_parameters takes care of that.
>>>> It does?  *Blink*.
>>> Well, something does. I can't say that I 100% understand all
>>> of how the new mount code handles the mount options. Y'all made
>>> sweeping changes, and the code works the way it used to except
>>> for the awkward change from smackfsdef to smackfsdefault. It
>>> took no small amount of head scratching and experimentation to
>>> convince myself that the fix I proposed was correct.
>> Ah...  I suspect the issue is that smack_sb_eat_lsm_opts() strips the prefix
>> for an unconverted filesystem, but smack_fs_context_parse_param() doesn't
>> (which it shouldn't).
>>
>> Can you try grabbing my mount-api-viro branch from:
>>
>> 	https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git
>>
>> and testing setting smack options on a tmpfs filesystem?
> My fedora system won't boot because smackfsdef isn't recognized. :(
> I will put in my fix and retry.

No joy there, either. Now it accepts smackfsdef, but doesn't
recognize smackfsroot. I don't have this problem with vanilla
5.1.

>
>> You might need to try modifying samples/vfs/test-fsmount.c to make it mount a
>> trmpfs filesystem through the new mount UAPI.
>>
>> David