Re: [PATCH 1/6] statmount: don't copy the reserved empty string past the buffer

[Jan Kara]

On Wed 21-10-26 13:46:54, Christian Brauner wrote:
> statmount_string() reserves an empty string at the start of the seq
> buffer before it calls the handler for the requested string. That's
> what the offsets of unset strings point to since commit e52e97f09fb6
> ("statmount: let unset strings be empty").
> 
> If the handler emits nothing the early return from commit 75ead69a7173
> ("fs: don't let statmount return empty strings") skips the size check
> against the caller's buffer. Back then an empty emission meant an empty
> seq buffer and there was nothing to copy. Now the reserved NUL byte is
> there and copy_statmount_to_user() copies it to buf + sizeof(struct
> statmount) no matter how small bufsize is.
> 
> So request a string that may turn out empty with a buffer that's
> smaller than struct statmount and the kernel writes one zero byte past
> the caller's buffer and reports a size that's larger than the buffer.
> STATMOUNT_MNT_UIDMAP or STATMOUNT_MNT_GIDMAP on a mount that isn't
> idmapped will do, so will STATMOUNT_FS_SUBTYPE on anything but fuse or
> STATMOUNT_OPT_ARRAY on a filesystem without ->show_options(). None of
> this needs privileges. The byte only lands in the caller's own address
> space but that's still not ok.
> 
> prepare_kstatmount() already refuses a buffer of exactly sizeof(struct
> statmount) for string requests. A smaller buffer can't hold a string
> either and it can't even hold the reserved empty string. So refuse any
> buffer that isn't larger than the fixed part. That's also what the uapi
> header says: string requests need space after the fixed part of the
> structure.
> 
> Reproducer, unprivileged, on any mount that isn't idmapped:
> 
>     #define _GNU_SOURCE
>     #include <errno.h>
>     #include <fcntl.h>
>     #include <stdint.h>
>     #include <stdio.h>
>     #include <string.h>
>     #include <sys/mman.h>
>     #include <sys/syscall.h>
>     #include <unistd.h>
>     #define STATMOUNT_FS_SUBTYPE	0x00000100U
>     #define STATMOUNT_MNT_UIDMAP	0x00002000U
>     #define STATX_MNT_ID_UNIQUE	0x00004000U
>     #define SM_SIZE			512 /* sizeof(struct statmount) */
>     struct mnt_id_req {
>     	uint32_t size, mnt_ns_fd;
>     	uint64_t mnt_id, param, mnt_ns_id;
>     };
>     static uint64_t root_mnt_id(void)
>     {
>     	unsigned char stx[256]; /* struct statx, stx_mnt_id at offset 144 */
>     	uint64_t id;
>     	if (syscall(__NR_statx, AT_FDCWD, "/", 0, STATX_MNT_ID_UNIQUE, stx))
>     		_exit(1);
>     	memcpy(&id, stx + 144, sizeof(id));
>     	return id;
>     }
>     static void probe(uint64_t mnt_id, uint64_t mask, int guard)
>     {
>     	struct mnt_id_req req = {
>     		.size = sizeof(req), .mnt_id = mnt_id, .param = mask,
>     	};
>     	long page = sysconf(_SC_PAGESIZE);
>     	unsigned char *map, *buf;
>     	uint32_t size;
>     	long ret;
>     	map = mmap(NULL, 2 * page, PROT_READ | PROT_WRITE,
>     		   MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
>     	memset(map, 0xaa, 2 * page);
>     	/* 100 byte buffer ending right at the page boundary */
>     	buf = map + page - 100;
>     	if (guard)
>     		mprotect(map + page, page, PROT_NONE);
>     	errno = 0;
>     	ret = syscall(__NR_statmount, &req, buf, 100, 0);
>     	printf("mask 0x%04llx%s: ret %ld errno %d", (unsigned long long)mask,
>     	       guard ? " guard page" : "", ret, errno);
>     	if (!guard) {
>     		memcpy(&size, buf, sizeof(size));
>     		printf(", size 0x%x, byte[%d] 0x%02x", size, SM_SIZE,
>     		       buf[SM_SIZE]);
>     	}
>     	printf("\n");
>     	munmap(map, 2 * page);
>     }
>     int main(void)
>     {
>     	uint64_t id = root_mnt_id();
>     	probe(id, STATMOUNT_MNT_UIDMAP, 0);
>     	probe(id, STATMOUNT_FS_SUBTYPE, 0);
>     	probe(id, STATMOUNT_MNT_UIDMAP, 1);
>     	return 0;
>     }
> 
> Before, on 7.3.0-rc7-00252-g4e3259b08c54 (vfs.fixes) in a VM, the
> same on a distro 7.1.13:
> 
>     mask 0x2000: ret 0 errno 0, size 0x65, byte[512] 0x00
>     mask 0x0100: ret 0 errno 0, size 0x65, byte[512] 0x00
>     mask 0x2000 guard page: ret -1 errno 14
> 
> After:
> 
>     mask 0x2000: ret -1 errno 75, size 0xaaaaaaaa, byte[512] 0xaa
>     mask 0x0100: ret -1 errno 75, size 0xaaaaaaaa, byte[512] 0xaa
>     mask 0x2000 guard page: ret -1 errno 75
> 
> Reported-by: shaos <shawnq1@xxxxxxx>
> Link: https://lore.kernel.org/r/20261020110747.959829-1-shawnq1@xxxxxxx
> Fixes: e52e97f09fb6 ("statmount: let unset strings be empty")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Christian Brauner (Amutable) <brauner@xxxxxxxxxx>

Looks good but FWIW I've already reviewed the same fix here:

https://lore.kernel.org/all/e6ysb6lyld5remaojkqqbaffvlsxwhr4tmurifp7mhbqrj7l54@6yqqwu3d7e3y

Feel free to add:

Reviewed-by: Jan Kara <jack@xxxxxxx>

									Honza

> ---
>  fs/namespace.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/namespace.c b/fs/namespace.c
> index c30f063742f4..6c9c0cdbe890 100644
> --- a/fs/namespace.c
> +++ b/fs/namespace.c
> @@ -6058,7 +6058,8 @@ static int prepare_kstatmount(struct kstatmount *ks, struct mnt_id_req *kreq,
>  	ks->bufsize = bufsize;
>  
>  	if (ks->mask & STATMOUNT_STRING_REQ) {
> -		if (bufsize == sizeof(ks->sm))
> +		/* Even the reserved empty string needs room past the fixed part. */
> +		if (bufsize <= sizeof(ks->sm))
>  			return -EOVERFLOW;
>  
>  		ks->seq.buf = kvmalloc(seq_size, GFP_KERNEL_ACCOUNT);
> 
> -- 
> 2.53.0
> 
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR