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@xxxxxxxxxx>
> ---
>  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@xxxxxxxx>
SUSE Labs, CR