On Fri, Jun 5, 2020 at 6:43 AM Bart Van Assche <bvanassche@xxxxxxx> wrote:
>
> On 2020-06-04 22:02, harshad shirwadkar wrote:
> > Hi Bart,
> >
> > On Thu, Jun 4, 2020 at 9:31 PM Bart Van Assche <bvanassche@xxxxxxx> wrote:
> >>
> >> 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?
> >
> > From what I understand, these are not limits. These are hardcoded
> > values used by blktrace userspace when it calls BLKTRACESETUP ioctl.
> > Inside the kernel, before this patch there is no checking in the linux
> > kernel.
>
> Please do not move limits from userspace tools into the kernel. Doing so
> would break other user space software (not sure if there is any) that
> uses the same ioctl but different limits.
Ack.
>
> >>> 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.
> >
> > Thanks, makes sense. I'll do this in the V2. If we do that, we still
> > need to define the default values for these limits. Do the values
> > chosen for MAX_BUFSIZE and MAX_ALLOC look reasonable?
>
> We typically do not implement arbitrary limits in the kernel. So I'd
> prefer not to introduce any artificial limits.
I see. But my worry is that if we don't check for bounds in the kernel
in this case, a non-root user who has access to any block device (even
a simple loop device) can make the kernel go out of memory.
---
...
int main(int argc, char *argv[])
{
struct blk_user_trace_setup buts;
int fd;
int ret;
fd = open(argv[1], O_RDONLY|O_NONBLOCK);
memset(&buts, 0, sizeof(buts));
buts.buf_size = ~0;
buts.buf_nr = 1;
buts.act_mask = 65535;
return ioctl(fd, BLKTRACESETUP, &buts);
}
---
I understand that introducing artificial hard-coded limits is probably
not the best approach. However, not having a reasonable sanity
checking on the IOCTL arguments, especially when the IOCTL itself
doesn't require any special capability checks and the kernel uses
those arguments almost as-is for allocating memory seems like a
vulnerability that attackers can exploit.
>
> >>
> >>> /*
> >>> * 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);
> >
> > Good catch, I'm not sure if the overflows are checked for, but I maybe
> > wrong. Given the possibility of overflows, perhaps we should be
> > checking for individual upper bounds instead of the product?
>
> How about using check_mul_overflow() to catch overflows of this
> multiplication? There may be other statements that need protection
> against overflow.
Makes sense. I can add that check.
Thanks,
Harshad.
>
> Thanks,
>
> Bart.
