On 6/3/20 10:45 PM, 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)
>
> We add buffer to this and define the upper bound to be as follows:
> BUF_SIZE=(1024 * 1024)
> BUF_NR=(16)
>
Aren't these values should be system dependent to start with?
I wonder what are the side effects of having hard-coded values ?
> This is very easy to exploit. Setting buf_size / buf_nr in userspace
> program to big values make kernel go oom. Verified that the fix makes
> BLKTRACESETUP return -E2BIG if the buf_size * buf_nr crosses the upper
> bound.
>
> Signed-off-by: Harshad Shirwadkar <harshadshirwadkar@xxxxxxxxx>
> ---
> include/uapi/linux/blktrace_api.h | 3 +++
> kernel/trace/blktrace.c | 3 +++
> 2 files changed, 6 insertions(+)
>
> 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))
>
This is an API change and should be reflected to kernel & userspace
tools. One way of doing it is to remove BUF_SIZE and BUF_NR and sync
kernel header with userspace blktrace_api.h.
> /*
> * 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;
> +
On the other hand we can easily get rid of the kernel part by moving
this check into user space tools, this will minimize the change diff and
we will still have sane behavior.
What about something like this (completely untested/not compiled) :-
diff --git a/blktrace.c b/blktrace.c
index d0d271f..d6a9f39 100644
--- a/blktrace.c
+++ b/blktrace.c
@@ -2247,6 +2247,9 @@ static int handle_args(int argc, char *argv[])
if (net_mode == Net_client && net_setup_addr())
return 1;
+ /* Check for unsually large numbers for buffers */
+ if (buf_size * buf_nr > BLKTRACE_MAX_ALLOC)
+ return -E2BIG;
/*
* Set up for appropriate PFD handler based upon output name.
*/
> if (!blk_debugfs_root)
> return -ENOENT;
>
>
