Re: [PATCH 07/17] fsinfo: Add fsinfo() syscall to query filesystem information [ver #17]

[David Howells <dhowells@redhat.com>]

Aleksa Sarai <cyphar@cyphar.com> wrote:

> > If params is given, all of params->__reserved[] must be 0.
> 
> I would suggest that rather than having a reserved field for future
> extensions, you make use of copy_struct_from_user() and have extensible
> structs:

Yeah.  I seem to recall that special support was required for 6-arg syscalls
on some arches, though I could move the dfd argument into the parameter block
and make AT_FDCWD the default.

> I dropped the "const" on fsinfo_params because the planned CHECK_FiELDS
> feature for extensible-struct syscalls requires writing to the struct.

Ummm...  Why?  You shouldn't be trying to alter the parameters structure.  It
could feasibly be stored static const in userspace (though I'm not sure how
likely it would be that someone would do that).

> I also switched the flags field to u64 because CHECK_FiELDS is intended to
> use (1<<63) for all syscalls (this has the nice benefit of removing the need
> of a padding field entirely).

 	struct fsinfo_params {
 		__u32	flags;
 		__u32	at_flags;
 		__u32	request;
 		__u32	Nth;
 		__u32	Mth;
 	};

What padding? ;-)

Though possibly the struct does need forcing to 64-bit alignment for future
expansion.

> > dfd, filename and params->at_flags indicate the file to query.  There is no
> > equivalent of lstat() as that can be emulated with fsinfo() by setting
> > AT_SYMLINK_NOFOLLOW in params->at_flags.
> 
> Minor gripe -- can we make the default be AT_SYMLINK_NOFOLLOW and you
> need to explicitly pass AT_SYMLINK_FOLLOW? Accidentally following
> symlinks is a constant source of security bugs.

Someone else has said that all new syscalls should be using RESOLVE_* flags in
preference to AT_* flags (even though RESOLVE_* flags are not a superset of
AT_* flags and appear to be in a header named specifically for the openat2()
syscall, not generic).

I'm not sure who authored openat2.h, but they went with a RESOLVE_NO_SYMLINKS
rather than a RESOLVE_SYMLINKS ;-)

> > There is also no equivalent of fstat() as that can be emulated by
> > passing a NULL filename to fsinfo() with the fd of interest in dfd.
> 
> Presumably you also need to pass AT_EMPTY_PATH?

Actually, you need to set FSINFO_FLAGS_QUERY_FD in fsinfo_params::flags.  I
need to update the description for this.

> Sounds good, though I think we should zero-fill the tail end of the
> buffer (if the buffer is larger than the in-kernel one).

I do that.  I should make it clearer in the patch description.

David