[PATCH] statx: reject unknown flags when using NULL path

[PATCH] statx: reject unknown flags when using NULL path

[Eric Biggers]

From: Eric Biggers <ebiggers-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>

The statx() system call currently accepts unknown flags when called with
a NULL path to operate on a file descriptor.  Left unchanged, this could
make it hard to introduce new query flags in the future, since
applications may not be able to tell whether a given flag is supported.

Fix this by failing the system call with EINVAL if any flags other than
KSTAT_QUERY_FLAGS are specified in combination with a NULL path.

Arguably, we could still permit known lookup-related flags such as
AT_SYMLINK_NOFOLLOW.  However, that would be inconsistent with how
sys_utimensat() behaves when passed a NULL path, which seems to be the
closest precedent.  And given that the NULL path case is (I believe)
mainly intended to be used to implement a wrapper function like fstatx()
that doesn't have a path argument, I think rejecting lookup-related
flags too is probably the best choice.

Signed-off-by: Eric Biggers <ebiggers-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
---
 fs/stat.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/fs/stat.c b/fs/stat.c
index fa0be59340cc..df484a60846d 100644
--- a/fs/stat.c
+++ b/fs/stat.c
@@ -130,9 +130,13 @@ EXPORT_SYMBOL(vfs_getattr);
 int vfs_statx_fd(unsigned int fd, struct kstat *stat,
 		 u32 request_mask, unsigned int query_flags)
 {
-	struct fd f = fdget_raw(fd);
+	struct fd f;
 	int error = -EBADF;
 
+	if (query_flags & ~KSTAT_QUERY_FLAGS)
+		return -EINVAL;
+
+	f = fdget_raw(fd);
 	if (f.file) {
 		error = vfs_getattr(&f.file->f_path, stat,
 				    request_mask, query_flags);
-- 
2.12.0

[PATCH] statx: reject unknown flags when using NULL path

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

The statx() system call currently accepts unknown flags when called with
a NULL path to operate on a file descriptor.  Left unchanged, this could
make it hard to introduce new query flags in the future, since
applications may not be able to tell whether a given flag is supported.

Fix this by failing the system call with EINVAL if any flags other than
KSTAT_QUERY_FLAGS are specified in combination with a NULL path.

Arguably, we could still permit known lookup-related flags such as
AT_SYMLINK_NOFOLLOW.  However, that would be inconsistent with how
sys_utimensat() behaves when passed a NULL path, which seems to be the
closest precedent.  And given that the NULL path case is (I believe)
mainly intended to be used to implement a wrapper function like fstatx()
that doesn't have a path argument, I think rejecting lookup-related
flags too is probably the best choice.

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 fs/stat.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/fs/stat.c b/fs/stat.c
index fa0be59340cc..df484a60846d 100644
--- a/fs/stat.c
+++ b/fs/stat.c
@@ -130,9 +130,13 @@ EXPORT_SYMBOL(vfs_getattr);
 int vfs_statx_fd(unsigned int fd, struct kstat *stat,
 		 u32 request_mask, unsigned int query_flags)
 {
-	struct fd f = fdget_raw(fd);
+	struct fd f;
 	int error = -EBADF;
 
+	if (query_flags & ~KSTAT_QUERY_FLAGS)
+		return -EINVAL;
+
+	f = fdget_raw(fd);
 	if (f.file) {
 		error = vfs_getattr(&f.file->f_path, stat,
 				    request_mask, query_flags);
-- 
2.12.0

Re: [PATCH] statx: reject unknown flags when using NULL path

[Eric Biggers]

On Fri, Mar 10, 2017 at 10:58:23PM -0800, Eric Biggers wrote:
> From: Eric Biggers <ebiggers@google.com>
> 
> The statx() system call currently accepts unknown flags when called with
> a NULL path to operate on a file descriptor.  Left unchanged, this could
> make it hard to introduce new query flags in the future, since
> applications may not be able to tell whether a given flag is supported.
> 
> Fix this by failing the system call with EINVAL if any flags other than
> KSTAT_QUERY_FLAGS are specified in combination with a NULL path.
> 

Does anyone have comments on this patch?

I really think we need to get this in before v4.11 is released, since it deals
with the API.

- Eric

Re: [PATCH] statx: reject unknown flags when using NULL path

[Christoph Hellwig]

On Mon, Mar 20, 2017 at 10:11:42AM -0700, Eric Biggers wrote:
> On Fri, Mar 10, 2017 at 10:58:23PM -0800, Eric Biggers wrote:
> > From: Eric Biggers <ebiggers@google.com>
> > 
> > The statx() system call currently accepts unknown flags when called with
> > a NULL path to operate on a file descriptor.  Left unchanged, this could
> > make it hard to introduce new query flags in the future, since
> > applications may not be able to tell whether a given flag is supported.
> > 
> > Fix this by failing the system call with EINVAL if any flags other than
> > KSTAT_QUERY_FLAGS are specified in combination with a NULL path.
> > 
> 
> Does anyone have comments on this patch?

Looks good to me.

> I really think we need to get this in before v4.11 is released, since it deals
> with the API.

Yes.  And we really need test for all of this.  Or just revert the
patches.  Shipping with untested syscalls is just a desaster.

Re: [PATCH] statx: reject unknown flags when using NULL path

[Christoph Hellwig]

Al, Linus:

can we get this in please?  It would be sad to grow another syscall
with an unchecked flags argument..

Re: [PATCH] statx: reject unknown flags when using NULL path

[Al Viro]

On Fri, Mar 31, 2017 at 08:39:52AM -0700, Christoph Hellwig wrote:
> Al, Linus:
> 
> can we get this in please?  It would be sad to grow another syscall
> with an unchecked flags argument..

Applied, will be in tonight pull request...

Re: [PATCH] statx: reject unknown flags when using NULL path

[David Howells]

Al Viro <viro-3bDd1+5oDREiFSDQTTA3OLVCufUGDwFn@public.gmane.org> wrote:

> > can we get this in please?  It would be sad to grow another syscall
> > with an unchecked flags argument..
> 
> Applied, will be in tonight pull request...

I'm not convinced that this is right.  I'm more inclined to let any flag be
passed that is available to statx() with a filename and just mask off the
bits before handing them on.

David

Re: [PATCH] statx: reject unknown flags when using NULL path

[David Howells]

Al Viro <viro@ZenIV.linux.org.uk> wrote:

> > can we get this in please?  It would be sad to grow another syscall
> > with an unchecked flags argument..
> 
> Applied, will be in tonight pull request...

I'm not convinced that this is right.  I'm more inclined to let any flag be
passed that is available to statx() with a filename and just mask off the
bits before handing them on.

David

Re: [PATCH] statx: reject unknown flags when using NULL path

[Christoph Hellwig]

On Fri, Mar 31, 2017 at 04:51:24PM +0100, David Howells wrote:
> I'm not convinced that this is right.  I'm more inclined to let any flag be
> passed that is available to statx() with a filename and just mask off the
> bits before handing them on.

And what would these flags actually do?  Currently we verify that
the allowed flags are passed for the case where we have a filename.

We need to do the same for the non-filename case, and we should
also check that we don't pass flags that don't make sense for this case.

Eric's patch is doing exactly that.

The only thing he could do even better is to add a testcase to xfstests
to verify this :)

Re: [PATCH] statx: reject unknown flags when using NULL path

[Christoph Hellwig]

On Fri, Mar 31, 2017 at 04:51:24PM +0100, David Howells wrote:
> I'm not convinced that this is right.  I'm more inclined to let any flag be
> passed that is available to statx() with a filename and just mask off the
> bits before handing them on.

And what would these flags actually do?  Currently we verify that
the allowed flags are passed for the case where we have a filename.

We need to do the same for the non-filename case, and we should
also check that we don't pass flags that don't make sense for this case.

Eric's patch is doing exactly that.

The only thing he could do even better is to add a testcase to xfstests
to verify this :)

Re: [PATCH] statx: reject unknown flags when using NULL path

[David Howells]

Christoph Hellwig <hch-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org> wrote:

> The only thing he could do even better is to add a testcase to xfstests
> to verify this :)

I will be pushing this sort of thing to LTP.

David

Re: [PATCH] statx: reject unknown flags when using NULL path

[David Howells]

Christoph Hellwig <hch@infradead.org> wrote:

> The only thing he could do even better is to add a testcase to xfstests
> to verify this :)

I will be pushing this sort of thing to LTP.

David