[PULL] Smack: Restore the smackfsdef mount option

[PULL] Smack: Restore the smackfsdef mount option

[Casey Schaufler]

James, this is a repair for a regression introduced in 5.1.
It should be pulled for 5.2 and added to 5.1.

The following changes since commit 619ae03e922b65a1a5d4269ceae1e9e13a058d6b:

  Smack: Fix kbuild reported build error (2019-04-30 14:13:32 -0700)

are available in the git repository at:

  https://github.com/cschaufler/next-smack.git smack-for-5.2-b

for you to fetch changes up to a5765ce797070d046dc53ccceeb0ed304cb918eb:

  Smack: Restore the smackfsdef mount option (2019-05-28 10:22:04 -0700)

----------------------------------------------------------------
Casey Schaufler (1):
      Smack: Restore the smackfsdef mount option

 security/smack/smack_lsm.c | 2 ++
 1 file changed, 2 insertions(+)

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

[David Howells]

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

> James, this is a repair for a regression introduced in 5.1.
> It should be pulled for 5.2 and added to 5.1.
> 
> The following changes since commit 619ae03e922b65a1a5d4269ceae1e9e13a058d6b:
> 
>   Smack: Fix kbuild reported build error (2019-04-30 14:13:32 -0700)
> 
> are available in the git repository at:
> 
>   https://github.com/cschaufler/next-smack.git smack-for-5.2-b
> 
> for you to fetch changes up to a5765ce797070d046dc53ccceeb0ed304cb918eb:
> 
>   Smack: Restore the smackfsdef mount option (2019-05-28 10:22:04 -0700)

Can you hold this for the moment, please?

Note that there appears to be another problem by inspection of the code.  I
think that smack_sb_eat_lsm_opts() strips the "smack" prefix off of the
options, whereas smack_fs_context_parse_param() does not.

This means that there's no need to do this:

	 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),

but that all the option names in that table *do* need prefixing with "smack".

The way you enter the LSM is going to depend on whether
generic_parse_monolithic() is called.  You're only going to enter this way if
mount(2) is the syscall of entry and the filesystem doesn't override the
->parse_monolithic() option (none in the upstream kernel).

David

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

[Casey Schaufler]

On 5/28/2019 12:02 PM, David Howells wrote:
> Casey Schaufler <casey@schaufler-ca.com> wrote:
>
>> James, this is a repair for a regression introduced in 5.1.
>> It should be pulled for 5.2 and added to 5.1.
>>
>> The following changes since commit 619ae03e922b65a1a5d4269ceae1e9e13a058d6b:
>>
>>   Smack: Fix kbuild reported build error (2019-04-30 14:13:32 -0700)
>>
>> are available in the git repository at:
>>
>>   https://github.com/cschaufler/next-smack.git smack-for-5.2-b
>>
>> for you to fetch changes up to a5765ce797070d046dc53ccceeb0ed304cb918eb:
>>
>>   Smack: Restore the smackfsdef mount option (2019-05-28 10:22:04 -0700)
> Can you hold this for the moment, please?

OK ...

> Note that there appears to be another problem by inspection of the code.  I
> think that smack_sb_eat_lsm_opts() strips the "smack" prefix off of the
> options, whereas smack_fs_context_parse_param() does not.
>
> This means that there's no need to do this:
>
> 	 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),
>
> but that all the option names in that table *do* need prefixing with "smack".

I'm not sure I follow the logic, because "mount -o smackfsdefault=Pop"
does what I would expect it to.

> The way you enter the LSM is going to depend on whether
> generic_parse_monolithic() is called.  You're only going to enter this way if
> mount(2) is the syscall of entry and the filesystem doesn't override the
> ->parse_monolithic() option (none in the upstream kernel).

So you're saying that the code works for the mount(2) case,
but won't work for some other case? Are you planning a fix?
Will that fix include restoration of smackfsdef?

> David

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

[David Howells]

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

> > 	 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),
> >
> > but that all the option names in that table *do* need prefixing with
> > "smack".

Actually, you're right, we do need to add that *and* prefix it with "smack".

> I'm not sure I follow the logic, because "mount -o smackfsdefault=Pop"
> does what I would expect it to.

Yes, I'm sure it does - for the cases you're testing - but it's filesystem and
syscall dependent.  None of the filesystems currently ported to the mount API
upstream override ->parse_monolithic(), but that changes with nfs, shmem and
coda and will change with cifs too.

It also changes if you use fsconfig() to supply the options because that goes
through a different LSM hook (it uses fs_context_parse_param rather than
sb_eat_lsm_opts).

> > The way you enter the LSM is going to depend on whether
> > generic_parse_monolithic() is called.  You're only going to enter this way
> > if mount(2) is the syscall of entry and the filesystem doesn't override
> > the ->parse_monolithic() option (none in the upstream kernel).
> 
> So you're saying that the code works for the mount(2) case,
> but won't work for some other case? Are you planning a fix?
> Will that fix include restoration of smackfsdef?

I can do a fix, but testing it is a pain.

David

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

[Casey Schaufler]

On 5/28/2019 1:18 PM, David Howells wrote:
> Casey Schaufler <casey@schaufler-ca.com> wrote:
>
>>> 	 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),
>>>
>>> but that all the option names in that table *do* need prefixing with
>>> "smack".
> Actually, you're right, we do need to add that *and* prefix it with "smack".
>
>> I'm not sure I follow the logic, because "mount -o smackfsdefault=Pop"
>> does what I would expect it to.
> Yes, I'm sure it does - for the cases you're testing - but it's filesystem and
> syscall dependent.  None of the filesystems currently ported to the mount API
> upstream override ->parse_monolithic(), but that changes with nfs, shmem and
> coda and will change with cifs too.
>
> It also changes if you use fsconfig() to supply the options because that goes
> through a different LSM hook (it uses fs_context_parse_param rather than
> sb_eat_lsm_opts).
>
>>> The way you enter the LSM is going to depend on whether
>>> generic_parse_monolithic() is called.  You're only going to enter this way
>>> if mount(2) is the syscall of entry and the filesystem doesn't override
>>> the ->parse_monolithic() option (none in the upstream kernel).
>> So you're saying that the code works for the mount(2) case,
>> but won't work for some other case? Are you planning a fix?
>> Will that fix include restoration of smackfsdef?
> I can do a fix, but testing it is a pain.

I will test a fix if you point me to it. I need it for 5.1 and 5.2.

>
> David