Re: [PATCH] blktrace: put bounds on BLKTRACESETUP buf_size and buf_nr

[Bart Van Assche <bvanassche@acm.org>]

On 2020-06-03 22:44, Harshad Shirwadkar wrote:
> Make sure that user requested memory via BLKTRACESETUP is within
> bounds. This can be easily exploited by setting really large values
> for buf_size and buf_nr in BLKTRACESETUP ioctl.
> 
> blktrace program has following hardcoded values for bufsize and bufnr:
> BUF_SIZE=(512 * 1024)
> BUF_NR=(4)

Where is the code that imposes these limits? I haven't found it. Did I
perhaps overlook something?

> diff --git a/include/uapi/linux/blktrace_api.h b/include/uapi/linux/blktrace_api.h
> index 690621b610e5..4d9dc44a83f9 100644
> --- a/include/uapi/linux/blktrace_api.h
> +++ b/include/uapi/linux/blktrace_api.h
> @@ -129,6 +129,9 @@ enum {
>  };
>  
>  #define BLKTRACE_BDEV_SIZE	32
> +#define BLKTRACE_MAX_BUFSIZ	(1024 * 1024)
> +#define BLKTRACE_MAX_BUFNR	16
> +#define BLKTRACE_MAX_ALLOC	((BLKTRACE_MAX_BUFNR) * (BLKTRACE_MAX_BUFNR))

Adding these constants to the user space API is a very inflexible
approach. There must be better approaches to export constants like these
to user space, e.g. through sysfs attributes.

It probably will be easier to get a patch like this one upstream if the
uapi headers are not modified.

>  /*
>   * User setup structure passed with BLKTRACESETUP
> diff --git a/kernel/trace/blktrace.c b/kernel/trace/blktrace.c
> index ea47f2084087..b3b0a8164c05 100644
> --- a/kernel/trace/blktrace.c
> +++ b/kernel/trace/blktrace.c
> @@ -482,6 +482,9 @@ static int do_blk_trace_setup(struct request_queue *q, char *name, dev_t dev,
>  	if (!buts->buf_size || !buts->buf_nr)
>  		return -EINVAL;
>  
> +	if (buts->buf_size * buts->buf_nr > BLKTRACE_MAX_ALLOC)
> +		return -E2BIG;
> +
>  	if (!blk_debugfs_root)
>  		return -ENOENT;

Where do the overflow(s) happen if buts->buf_size and buts->buf_nr are
large? Is the following code from relay_open() the only code that can
overflow?

	chan->alloc_size = PAGE_ALIGN(subbuf_size * n_subbufs);

Thanks,

Bart.