On Thu, Jun 4, 2020 at 12:10 AM Chaitanya Kulkarni
<Chaitanya.Kulkarni@xxxxxxx> wrote:
>
> 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 ?
Sure we can have system dependent bounds. But, the goal here is just
only to ensure that the caller doesn't call BLKTRACESETUP with
unreasonable values and potentially crash the kernel. Given that, do
we really need to have upper-bound to be system dependent? wouldn't a
hard-coded but reasonable upper-bound be sufficient? As of now
blktrace also uses hardcoded 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.
Thanks, so we should make a change in userspace header file too.
> > /*
> > * 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;
> >
> >
The user-space fix would fix blktrace binary but it's still easy to
exploit this by writing a really quick program that calls
BLKTRACESETUP ioctl with unreasonably high values for buf_size and
buf_nr.
Thanks,
Harshad
>
