On Wed, Jul 11, 2018 at 12:30:44PM -0600, Jason Gunthorpe wrote:
> On Sun, Jul 08, 2018 at 01:50:21PM +0300, Leon Romanovsky wrote:
> > From: Leon Romanovsky <leonro@xxxxxxxxxxxx>
> >
> > User's supplied index is checked again total number of system pages,
> > but this number already includes num_static_sys_pages, so addition
> > of that value to supplied index causes to below error while trying
> > to access sys_pages[].
> >
> > BUG: KASAN: slab-out-of-bounds in bfregn_to_uar_index+0x34f/0x400
> > Read of size 4 at addr ffff880065561904 by task syz-executor446/314
> >
> > CPU: 0 PID: 314 Comm: syz-executor446 Not tainted 4.18.0-rc1+ #256
> > Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.11.0-0-g63451fca13-prebuilt.qemu-project.org 04/01/2014
> > Call Trace:
> > dump_stack+0xef/0x17e
> > print_address_description+0x83/0x3b0
> > kasan_report+0x18d/0x4d0
> > bfregn_to_uar_index+0x34f/0x400
> > create_user_qp+0x272/0x227d
> > create_qp_common+0x32eb/0x43e0
> > mlx5_ib_create_qp+0x379/0x1ca0
> > create_qp.isra.5+0xc94/0x22d0
> > ib_uverbs_create_qp+0x21b/0x2a0
> > ib_uverbs_write+0xc2c/0x1010
> > vfs_write+0x1b0/0x550
> > ksys_write+0xc6/0x1a0
> > do_syscall_64+0xa7/0x590
> > entry_SYSCALL_64_after_hwframe+0x49/0xbe
> > RIP: 0033:0x433679
> > Code: fd ff 48 81 c4 80 00 00 00 e9 f1 fe ff ff 0f 1f 00 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 0f 83 3b 91 fd ff c3 66 2e 0f 1f 84 00 00 00 00
> > RSP: 002b:00007fff2b3d8e48 EFLAGS: 00000217 ORIG_RAX: 0000000000000001
> > RAX: ffffffffffffffda RBX: 00000000004002f8 RCX: 0000000000433679
> > RDX: 0000000000000040 RSI: 0000000020000240 RDI: 0000000000000003
> > RBP: 00000000006d4018 R08: 00000000004002f8 R09: 00000000004002f8
> > R10: 00000000004002f8 R11: 0000000000000217 R12: 0000000000000000
> > R13: 000000000040cb00 R14: 000000000040cb90 R15: 0000000000000006
> >
> > Allocated by task 314:
> > kasan_kmalloc+0xa0/0xd0
> > __kmalloc+0x1a9/0x510
> > mlx5_ib_alloc_ucontext+0x966/0x2620
> > ib_uverbs_get_context+0x23f/0xa60
> > ib_uverbs_write+0xc2c/0x1010
> > __vfs_write+0x10d/0x720
> > vfs_write+0x1b0/0x550
> > ksys_write+0xc6/0x1a0
> > do_syscall_64+0xa7/0x590
> > entry_SYSCALL_64_after_hwframe+0x49/0xbe
> >
> > Freed by task 1:
> > __kasan_slab_free+0x12e/0x180
> > kfree+0x159/0x630
> > kvfree+0x37/0x50
> > single_release+0x8e/0xf0
> > __fput+0x2d8/0x900
> > task_work_run+0x102/0x1f0
> > exit_to_usermode_loop+0x159/0x1c0
> > do_syscall_64+0x408/0x590
> > entry_SYSCALL_64_after_hwframe+0x49/0xbe
> >
> > The buggy address belongs to the object at ffff880065561100
> > which belongs to the cache kmalloc-4096 of size 4096
> > The buggy address is located 2052 bytes inside of
> > 4096-byte region [ffff880065561100, ffff880065562100)
> > The buggy address belongs to the page:
> > page:ffffea0001955800 count:1 mapcount:0 mapping:ffff88006c402480 index:0x0 compound_mapcount: 0
> > flags: 0x4000000000008100(slab|head)
> > raw: 4000000000008100 ffffea0001a7c000 0000000200000002 ffff88006c402480
> > raw: 0000000000000000 0000000080070007 00000001ffffffff 0000000000000000
> > page dumped because: kasan: bad access detected
> >
> > Memory state around the buggy address:
> > ffff880065561800: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> > ffff880065561880: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> > >ffff880065561900: 04 fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
> > ^
> > ffff880065561980: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
> > ffff880065561a00: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
> >
> > Cc: <stable@xxxxxxxxxxxxxxx> # 4.15
> > Fixes: 1ee47ab3e8d8 ("IB/mlx5: Enable QP creation with a given blue flame index")
> > Reported-by: Noa Osherovich <noaos@xxxxxxxxxxxx>
> > Signed-off-by: Leon Romanovsky <leonro@xxxxxxxxxxxx>
> > drivers/infiniband/hw/mlx5/mlx5_ib.h | 2 +-
> > drivers/infiniband/hw/mlx5/qp.c | 15 ++++++++-------
> > 2 files changed, 9 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/infiniband/hw/mlx5/mlx5_ib.h b/drivers/infiniband/hw/mlx5/mlx5_ib.h
> > index f427406bffc5..3337e3d289b1 100644
> > +++ b/drivers/infiniband/hw/mlx5/mlx5_ib.h
> > @@ -1361,6 +1361,6 @@ unsigned long mlx5_ib_get_xlt_emergency_page(void);
> > void mlx5_ib_put_xlt_emergency_page(void);
> >
> > int bfregn_to_uar_index(struct mlx5_ib_dev *dev,
> > - struct mlx5_bfreg_info *bfregi, int bfregn,
> > + struct mlx5_bfreg_info *bfregi, u32 bfregn,
> > bool dyn_bfreg);
> > #endif /* MLX5_IB_H */
> > diff --git a/drivers/infiniband/hw/mlx5/qp.c b/drivers/infiniband/hw/mlx5/qp.c
> > index c566dff6ee6e..4ac132ba19a7 100644
> > +++ b/drivers/infiniband/hw/mlx5/qp.c
> > @@ -631,22 +631,23 @@ static void mlx5_ib_unlock_cqs(struct mlx5_ib_cq *send_cq,
> > struct mlx5_ib_cq *recv_cq);
> >
> > int bfregn_to_uar_index(struct mlx5_ib_dev *dev,
> > - struct mlx5_bfreg_info *bfregi, int bfregn,
> > + struct mlx5_bfreg_info *bfregi, u32 bfregn,
> > bool dyn_bfreg)
> > {
> > - int bfregs_per_sys_page;
> > - int index_of_sys_page;
> > - int offset;
> > + unsigned int bfregs_per_sys_page;
> > + u32 index_of_sys_page;
> > + u32 offset;
> >
> > bfregs_per_sys_page = get_uars_per_sys_page(dev, bfregi->lib_uar_4k) *
> > MLX5_NON_FP_BFREGS_PER_UAR;
> > index_of_sys_page = bfregn / bfregs_per_sys_page;
> >
> > - if (index_of_sys_page >= bfregi->num_sys_pages)
> > - return -EINVAL;
> > -
> > if (dyn_bfreg) {
> > index_of_sys_page += bfregi->num_static_sys_pages;
> > +
> > + if (index_of_sys_page >= bfregi->num_sys_pages)
> > + return -EINVAL;
>
> This looks like the !dyn_bfreg path looses its range check though?
>
> Should it be like this?
No, "!dyn_bfreg" means this bfreg comes from kernel and not from user
and it is guaranteed to be in the proper range. The problem exists only
with user's bfreg indexes because it is first function which validates
its.
>
> @@ -653,16 +653,19 @@ int bfregn_to_uar_index(struct mlx5_ib_dev *dev,
> MLX5_NON_FP_BFREGS_PER_UAR;
> index_of_sys_page = bfregn / bfregs_per_sys_page;
>
> - if (index_of_sys_page >= bfregi->num_sys_pages)
> - return -EINVAL;
> -
> if (dyn_bfreg) {
> index_of_sys_page += bfregi->num_static_sys_pages;
> + if (index_of_sys_page >= bfregi->num_sys_pages)
> + return -EINVAL;
> +
> if (bfregn > bfregi->num_dyn_bfregs ||
> bfregi->sys_pages[index_of_sys_page] == MLX5_IB_INVALID_UAR_INDEX) {
> mlx5_ib_dbg(dev, "Invalid dynamic uar index\n");
> return -EINVAL;
> }
> + } else {
> + if (index_of_sys_page >= bfregi->num_sys_pages)
> + return -EINVAL;
> }
>
> offset = bfregn % bfregs_per_sys_page / MLX5_NON_FP_BFREGS_PER_UAR;
>
> Jason
Attachment:
signature.asc
Description: PGP signature
