[PATCH 0/3] pidfs: small fixes

[PATCH 0/3] pidfs: small fixes

[Christian Brauner]

Three independent pidfs bug fixes, each with a Fixes: tag.

Patch 1 fixes PIDFD_THREAD flag loss when pidfds are opened via file
handles. PIDFD_THREAD is defined as O_EXCL, and do_dentry_open() strips
O_EXCL from f_flags, so thread pidfds obtained via open_by_handle_at()
silently end up with PIDTYPE_TGID scope. pidfs_alloc_file() already
restored the flag after dentry_open(); factor that into a shared
pidfs_open_file() helper and use it from pidfs_export_open() too.
Without this, pidfd_send_signal() on a thread pidfd reopened from a
file handle delivers to the entire thread group instead of the
specific thread.

Patch 2 fixes pidfs_xattr_get() returning 0 when no xattrs have ever
been set (attr->xattrs == NULL). The VFS interprets 0 as "xattr exists
with a zero-length value", so getxattr() on a pidfd reports success
for non-existent xattrs. Return -ENODATA instead, matching
simple_xattr_get().

Patch 3 enforces the documented PIDFD_GET_INFO contract that the
kernel must not set a mask bit unless the user buffer is large enough
to carry the corresponding field. Today PIDFD_INFO_COREDUMP,
PIDFD_INFO_COREDUMP_SIGNAL and PIDFD_INFO_SUPPORTED_MASK are returned
in the mask without checking usize against PIDFD_INFO_SIZE_VER1/VER2.
copy_struct_to_user() stops at min(usize, ksize) so no kernel memory
leaks, but userspace that trusts the mask as documented will read its
own uninitialized buffer as if it were valid data. Gate the mask bits
on usize.

Signed-off-by: Christian Brauner (Amutable) <brauner@kernel.org>
---
Christian Brauner (3):
      pidfs: fix PIDFD_THREAD flag loss when opening pidfds via file handles
      pidfs: return -ENODATA from pidfs_xattr_get() when no xattrs exist
      pidfs: don't report pidfd_info fields that won't fit in the user buffer

 fs/pidfs.c | 38 +++++++++++++++++++++++---------------
 1 file changed, 23 insertions(+), 15 deletions(-)
---
base-commit: e774d5f1bc27a85f858bce7688509e866f8e8a4e
change-id: 20260420-work-pidfs-6152879f9434

[PATCH 1/3] pidfs: fix PIDFD_THREAD flag loss when opening pidfds via file handles

[Christian Brauner]

pidfs_export_open() calls dentry_open() which internally calls
do_dentry_open() that strips O_EXCL from f_flags. Since PIDFD_THREAD is
defined as O_EXCL, thread pidfds opened via open_by_handle_at() silently
lose their thread-specific scope.

pidfs_alloc_file() already handles this by explicitly restoring the
PIDFD_THREAD flag after dentry_open() returns. Factor the common
dentry_open() + flag restoration into a shared pidfs_open_file() helper
and use it from both call sites.

Without this fix, a pidfd obtained via open_by_handle_at() with O_EXCL
will have PIDTYPE_TGID scope instead of PIDTYPE_PID scope, causing
pidfd_send_signal() to deliver signals to the entire thread group
instead of the specific thread.

Fixes: 30915e955528 ("pidfs: convert to path_from_stashed() helper")
Signed-off-by: Christian Brauner <brauner@kernel.org>
---
 fs/pidfs.c | 29 ++++++++++++++++++-----------
 1 file changed, 18 insertions(+), 11 deletions(-)

diff --git a/fs/pidfs.c b/fs/pidfs.c
index e3825ee246be..11eb53f3e50a 100644
--- a/fs/pidfs.c
+++ b/fs/pidfs.c
@@ -897,14 +897,28 @@ static int pidfs_export_permission(struct handle_to_path_ctx *ctx,
 	return 0;
 }
 
+/*
+ * Open a pidfs dentry. Pidfds are always O_RDWR and PIDFD_THREAD (O_EXCL)
+ * must be restored after dentry_open() as do_dentry_open() strips it.
+ */
+static struct file *pidfs_open_file(const struct path *path, unsigned int flags)
+{
+	struct file *f;
+
+	flags |= O_RDWR;
+	f = dentry_open(path, flags, current_cred());
+	if (!IS_ERR(f))
+		f->f_flags |= (flags & PIDFD_THREAD);
+	return f;
+}
+
 static struct file *pidfs_export_open(const struct path *path, unsigned int oflags)
 {
 	/*
-	 * Clear O_LARGEFILE as open_by_handle_at() forces it and raise
-	 * O_RDWR as pidfds always are.
+	 * Clear O_LARGEFILE as open_by_handle_at() forces it.
 	 */
 	oflags &= ~O_LARGEFILE;
-	return dentry_open(path, oflags | O_RDWR, current_cred());
+	return pidfs_open_file(path, oflags);
 }
 
 static const struct export_operations pidfs_export_operations = {
@@ -1086,7 +1100,6 @@ static struct file_system_type pidfs_type = {
 
 struct file *pidfs_alloc_file(struct pid *pid, unsigned int flags)
 {
-	struct file *pidfd_file;
 	struct path path __free(path_put) = {};
 	int ret;
 
@@ -1104,13 +1117,7 @@ struct file *pidfs_alloc_file(struct pid *pid, unsigned int flags)
 	VFS_WARN_ON_ONCE(!pid->attr);
 
 	flags &= ~PIDFD_STALE;
-	flags |= O_RDWR;
-	pidfd_file = dentry_open(&path, flags, current_cred());
-	/* Raise PIDFD_THREAD explicitly as do_dentry_open() strips it. */
-	if (!IS_ERR(pidfd_file))
-		pidfd_file->f_flags |= (flags & PIDFD_THREAD);
-
-	return pidfd_file;
+	return pidfs_open_file(&path, flags);
 }
 
 void __init pidfs_init(void)

-- 
2.47.3

Re: [PATCH 1/3] pidfs: fix PIDFD_THREAD flag loss when opening pidfds via file handles

[Jan Kara]

On Mon 20-04-26 15:32:35, Christian Brauner wrote:
> pidfs_export_open() calls dentry_open() which internally calls
> do_dentry_open() that strips O_EXCL from f_flags. Since PIDFD_THREAD is
> defined as O_EXCL, thread pidfds opened via open_by_handle_at() silently
> lose their thread-specific scope.
> 
> pidfs_alloc_file() already handles this by explicitly restoring the
> PIDFD_THREAD flag after dentry_open() returns. Factor the common
> dentry_open() + flag restoration into a shared pidfs_open_file() helper
> and use it from both call sites.
> 
> Without this fix, a pidfd obtained via open_by_handle_at() with O_EXCL
> will have PIDTYPE_TGID scope instead of PIDTYPE_PID scope, causing
> pidfd_send_signal() to deliver signals to the entire thread group
> instead of the specific thread.
> 
> Fixes: 30915e955528 ("pidfs: convert to path_from_stashed() helper")
> Signed-off-by: Christian Brauner <brauner@kernel.org>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/pidfs.c | 29 ++++++++++++++++++-----------
>  1 file changed, 18 insertions(+), 11 deletions(-)
> 
> diff --git a/fs/pidfs.c b/fs/pidfs.c
> index e3825ee246be..11eb53f3e50a 100644
> --- a/fs/pidfs.c
> +++ b/fs/pidfs.c
> @@ -897,14 +897,28 @@ static int pidfs_export_permission(struct handle_to_path_ctx *ctx,
>  	return 0;
>  }
>  
> +/*
> + * Open a pidfs dentry. Pidfds are always O_RDWR and PIDFD_THREAD (O_EXCL)
> + * must be restored after dentry_open() as do_dentry_open() strips it.
> + */
> +static struct file *pidfs_open_file(const struct path *path, unsigned int flags)
> +{
> +	struct file *f;
> +
> +	flags |= O_RDWR;
> +	f = dentry_open(path, flags, current_cred());
> +	if (!IS_ERR(f))
> +		f->f_flags |= (flags & PIDFD_THREAD);
> +	return f;
> +}
> +
>  static struct file *pidfs_export_open(const struct path *path, unsigned int oflags)
>  {
>  	/*
> -	 * Clear O_LARGEFILE as open_by_handle_at() forces it and raise
> -	 * O_RDWR as pidfds always are.
> +	 * Clear O_LARGEFILE as open_by_handle_at() forces it.
>  	 */
>  	oflags &= ~O_LARGEFILE;
> -	return dentry_open(path, oflags | O_RDWR, current_cred());
> +	return pidfs_open_file(path, oflags);
>  }
>  
>  static const struct export_operations pidfs_export_operations = {
> @@ -1086,7 +1100,6 @@ static struct file_system_type pidfs_type = {
>  
>  struct file *pidfs_alloc_file(struct pid *pid, unsigned int flags)
>  {
> -	struct file *pidfd_file;
>  	struct path path __free(path_put) = {};
>  	int ret;
>  
> @@ -1104,13 +1117,7 @@ struct file *pidfs_alloc_file(struct pid *pid, unsigned int flags)
>  	VFS_WARN_ON_ONCE(!pid->attr);
>  
>  	flags &= ~PIDFD_STALE;
> -	flags |= O_RDWR;
> -	pidfd_file = dentry_open(&path, flags, current_cred());
> -	/* Raise PIDFD_THREAD explicitly as do_dentry_open() strips it. */
> -	if (!IS_ERR(pidfd_file))
> -		pidfd_file->f_flags |= (flags & PIDFD_THREAD);
> -
> -	return pidfd_file;
> +	return pidfs_open_file(&path, flags);
>  }
>  
>  void __init pidfs_init(void)
> 
> -- 
> 2.47.3
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH 2/3] pidfs: return -ENODATA from pidfs_xattr_get() when no xattrs exist

[Christian Brauner]

When no xattrs have ever been set on a pidfd (attr->xattrs is NULL),
pidfs_xattr_get() returns 0. The VFS interprets a return value of 0 as
"xattr exists with a zero-length value", causing getxattr() to report
success for non-existent xattrs.

Return -ENODATA instead which is consistent with what simple_xattr_get()
returns when an xattr is not found.

Fixes: 5c1e4b9db95c ("pidfs: support xattrs on pidfds")
Signed-off-by: Christian Brauner <brauner@kernel.org>
---
 fs/pidfs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/pidfs.c b/fs/pidfs.c
index 11eb53f3e50a..2ab8fd2646f0 100644
--- a/fs/pidfs.c
+++ b/fs/pidfs.c
@@ -1023,7 +1023,7 @@ static int pidfs_xattr_get(const struct xattr_handler *handler,
 
 	xattrs = READ_ONCE(attr->xattrs);
 	if (!xattrs)
-		return 0;
+		return -ENODATA;
 
 	name = xattr_full_name(handler, suffix);
 	return simple_xattr_get(xattrs, name, value, size);

-- 
2.47.3

Re: [PATCH 2/3] pidfs: return -ENODATA from pidfs_xattr_get() when no xattrs exist

[Jan Kara]

On Mon 20-04-26 15:32:36, Christian Brauner wrote:
> When no xattrs have ever been set on a pidfd (attr->xattrs is NULL),
> pidfs_xattr_get() returns 0. The VFS interprets a return value of 0 as
> "xattr exists with a zero-length value", causing getxattr() to report
> success for non-existent xattrs.
> 
> Return -ENODATA instead which is consistent with what simple_xattr_get()
> returns when an xattr is not found.
> 
> Fixes: 5c1e4b9db95c ("pidfs: support xattrs on pidfds")
> Signed-off-by: Christian Brauner <brauner@kernel.org>

Right. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/pidfs.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/pidfs.c b/fs/pidfs.c
> index 11eb53f3e50a..2ab8fd2646f0 100644
> --- a/fs/pidfs.c
> +++ b/fs/pidfs.c
> @@ -1023,7 +1023,7 @@ static int pidfs_xattr_get(const struct xattr_handler *handler,
>  
>  	xattrs = READ_ONCE(attr->xattrs);
>  	if (!xattrs)
> -		return 0;
> +		return -ENODATA;
>  
>  	name = xattr_full_name(handler, suffix);
>  	return simple_xattr_get(xattrs, name, value, size);
> 
> -- 
> 2.47.3
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH 3/3] pidfs: don't report pidfd_info fields that won't fit in the user buffer

[Christian Brauner]

The UAPI documentation for struct pidfd_info promises that if the
structure provided by userspace is too small to contain a field, the
kernel will not set the corresponding bit in the returned mask.

The kernel violates this contract: it sets PIDFD_INFO_COREDUMP and
PIDFD_INFO_COREDUMP_SIGNAL in the returned mask without checking that
usize >= PIDFD_INFO_SIZE_VER1 (the coredump fields start at offset 64,
beyond a VER0 buffer). Similarly, PIDFD_INFO_SUPPORTED_MASK is set
without checking usize >= PIDFD_INFO_SIZE_VER2.

While copy_struct_to_user() correctly only copies min(usize, ksize)
bytes (so no kernel memory leaks), userspace that trusts the mask bits
as documented may read its own uninitialized buffer and interpret it as
valid data.

Gate each set of mask bits on the user-provided struct being large
enough to actually deliver the corresponding fields.

Fixes: 9e77e4882bae ("pidfs: support retrieving supported pidfd_info flags")
Signed-off-by: Christian Brauner <brauner@kernel.org>
---
 fs/pidfs.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/fs/pidfs.c b/fs/pidfs.c
index 2ab8fd2646f0..4c24d2eb7e41 100644
--- a/fs/pidfs.c
+++ b/fs/pidfs.c
@@ -375,7 +375,7 @@ static long pidfd_info(struct file *file, unsigned int cmd, unsigned long arg)
 		}
 	}
 
-	if (mask & PIDFD_INFO_COREDUMP) {
+	if ((mask & PIDFD_INFO_COREDUMP) && usize >= PIDFD_INFO_SIZE_VER1) {
 		if (test_bit(PIDFS_ATTR_BIT_COREDUMP, &attr->attr_mask)) {
 			smp_rmb();
 			kinfo.mask |= PIDFD_INFO_COREDUMP | PIDFD_INFO_COREDUMP_SIGNAL;
@@ -400,7 +400,8 @@ static long pidfd_info(struct file *file, unsigned int cmd, unsigned long arg)
 	if (!c)
 		return -ESRCH;
 
-	if ((mask & PIDFD_INFO_COREDUMP) && !kinfo.coredump_mask) {
+	if ((mask & PIDFD_INFO_COREDUMP) && usize >= PIDFD_INFO_SIZE_VER1 &&
+	    !kinfo.coredump_mask) {
 		guard(task_lock)(task);
 		if (task->mm) {
 			unsigned long flags = __mm_flags_get_dumpable(task->mm);
@@ -455,7 +456,7 @@ static long pidfd_info(struct file *file, unsigned int cmd, unsigned long arg)
 		return -ESRCH;
 
 copy_out:
-	if (mask & PIDFD_INFO_SUPPORTED_MASK) {
+	if ((mask & PIDFD_INFO_SUPPORTED_MASK) && usize >= PIDFD_INFO_SIZE_VER2) {
 		kinfo.mask |= PIDFD_INFO_SUPPORTED_MASK;
 		kinfo.supported_mask = PIDFD_INFO_SUPPORTED;
 	}

-- 
2.47.3

Re: [PATCH 3/3] pidfs: don't report pidfd_info fields that won't fit in the user buffer

[Jan Kara]

On Mon 20-04-26 15:32:37, Christian Brauner wrote:
> The UAPI documentation for struct pidfd_info promises that if the
> structure provided by userspace is too small to contain a field, the
> kernel will not set the corresponding bit in the returned mask.
> 
> The kernel violates this contract: it sets PIDFD_INFO_COREDUMP and
> PIDFD_INFO_COREDUMP_SIGNAL in the returned mask without checking that
> usize >= PIDFD_INFO_SIZE_VER1 (the coredump fields start at offset 64,
> beyond a VER0 buffer). Similarly, PIDFD_INFO_SUPPORTED_MASK is set
> without checking usize >= PIDFD_INFO_SIZE_VER2.
> 
> While copy_struct_to_user() correctly only copies min(usize, ksize)
> bytes (so no kernel memory leaks), userspace that trusts the mask bits
> as documented may read its own uninitialized buffer and interpret it as
> valid data.
> 
> Gate each set of mask bits on the user-provided struct being large
> enough to actually deliver the corresponding fields.
> 
> Fixes: 9e77e4882bae ("pidfs: support retrieving supported pidfd_info flags")
> Signed-off-by: Christian Brauner <brauner@kernel.org>
> ---
>  fs/pidfs.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/pidfs.c b/fs/pidfs.c
> index 2ab8fd2646f0..4c24d2eb7e41 100644
> --- a/fs/pidfs.c
> +++ b/fs/pidfs.c
> @@ -375,7 +375,7 @@ static long pidfd_info(struct file *file, unsigned int cmd, unsigned long arg)
>  		}
>  	}
>  
> -	if (mask & PIDFD_INFO_COREDUMP) {
> +	if ((mask & PIDFD_INFO_COREDUMP) && usize >= PIDFD_INFO_SIZE_VER1) {
>  		if (test_bit(PIDFS_ATTR_BIT_COREDUMP, &attr->attr_mask)) {
>  			smp_rmb();
>  			kinfo.mask |= PIDFD_INFO_COREDUMP | PIDFD_INFO_COREDUMP_SIGNAL;

This sets also PIDFD_INFO_COREDUMP_CODE which only fits for VER2? I'm not
sure what's expected to happen for usize == VER1...

Otherwise the patch looks good.

									Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR