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.
