Re: [PATCH net-next v7 05/10] landlock: Add LSM hooks related to filesystem

[MickaÃl SalaÃn]

On 24/08/2017 04:50, Alexei Starovoitov wrote:
> On Mon, Aug 21, 2017 at 02:09:28AM +0200, Mickaël Salaün wrote:
>> Handle 33 filesystem-related LSM hooks for the Landlock filesystem
>> event: LANDLOCK_SUBTYPE_EVENT_FS.
>>
>> A Landlock event wrap LSM hooks for similar kernel object types (e.g.
>> struct file, struct path...). Multiple LSM hooks can trigger the same
>> Landlock event.
>>
>> Landlock handle nine coarse-grained actions: read, write, execute, new,
>> get, remove, ioctl, lock and fcntl. Each of them abstract LSM hook
>> access control in a way that can be extended in the future.
>>
>> The Landlock LSM hook registration is done after other LSM to only run
>> actions from user-space, via eBPF programs, if the access was granted by
>> major (privileged) LSMs.
>>
>> Signed-off-by: Mickaël Salaün <mic@xxxxxxxxxxx>
> 
> ...
> 
>> +/* WRAP_ARG_SB */
>> +#define WRAP_ARG_SB_TYPE	WRAP_TYPE_FS
>> +#define WRAP_ARG_SB_DEC(arg)					\
>> +	EXPAND_C(WRAP_TYPE_FS) wrap_##arg =			\
>> +	{ .type = BPF_HANDLE_FS_TYPE_DENTRY, .dentry = arg->s_root };
>> +#define WRAP_ARG_SB_VAL(arg)	((uintptr_t)&wrap_##arg)
>> +#define WRAP_ARG_SB_OK(arg)	(arg && arg->s_root)
> ...
> 
>> +HOOK_NEW_FS(sb_remount, 2,
>> +	struct super_block *, sb,
>> +	void *, data,
>> +	WRAP_ARG_SB, sb,
>> +	WRAP_ARG_RAW, LANDLOCK_ACTION_FS_WRITE
>> +);
> 
> this looks wrong. casting super_block to dentry?

This is called when remounting a block device. The WRAP_ARG_SB take the
sb->s_root as a dentry, it is not a cast. What do you expect from this hook?

> 
>> +/* a directory inode contains only one dentry */
>> +HOOK_NEW_FS(inode_create, 3,
>> +	struct inode *, dir,
>> +	struct dentry *, dentry,
>> +	umode_t, mode,
>> +	WRAP_ARG_INODE, dir,
>> +	WRAP_ARG_RAW, LANDLOCK_ACTION_FS_WRITE
>> +);
> 
> more general question: why you're not wrapping all useful
> arguments? Like in the above dentry can be acted upon
> by the landlock rule and it's readily available...

The context used for the FS event must have the exact same types for all
calls. This event is meant to be generic but we can add more specific
ones if needed, like I do with FS_IOCTL.

The idea is to enable people to write simple rules, while being able to
write fine grain rules for special cases (e.g. IOCTL) if needed.

> 
> The limitation of only 2 args looks odd.
> Is it a hard limitation ? how hard to extend?

It's not a hard limit at all. Actually, the FS_FNCTL event should have
three arguments (I'll add them in the next series): FS handle, FCNTL
command and FCNTL argument. I made sure that it's really easy to add
more arguments to the context of an event.

Attachment: signature.asc
Description: OpenPGP digital signature