[PATCH] nvme-core: remove head->effects to fix use-after-free

[Yuanyuan Zhong]

On Wed, Nov 15, 2023 at 11:02 AM Jens Axboe <axboe at kernel.dk> wrote:
>
> On 11/15/23 11:54 AM, Yuanyuan Zhong wrote:
> > The head->effects stores a pointer to the controller's Command Sets
> > Supported and Effects log. When the namespace head is shared by multiple
> > controllers, if the particular controller is removed, dereferencing
> > head->effects causes use-after-free:
> >
> > [  227.820066] ==================================================================
> > [  227.820069] BUG: KFENCE: use-after-free read in nvme_command_effects+0x1f/0x90 [nvme_core]
> >
> > [  227.820091] Use-after-free read at 0x00000000c02dadcf (in kfence-#0):
> > [  227.820094]  nvme_command_effects+0x1f/0x90 [nvme_core]
> > [  227.820107]  nvme_passthru_start+0x19/0x80 [nvme_core]
> > [  227.820121]  nvme_submit_user_cmd+0xc7/0x170 [nvme_core]
> > [  227.820136]  nvme_user_cmd.constprop.16+0x152/0x1d0 [nvme_core]
> > [  227.820149]  nvme_ns_head_ioctl+0x92/0x140 [nvme_core]
> > [  227.820162]  blkdev_ioctl+0x1c5/0x280
> > [  227.820169]  __x64_sys_ioctl+0x93/0xd0
> > [  227.820174]  do_syscall_64+0x45/0xf0
> > [  227.820177]  entry_SYSCALL_64_after_hwframe+0x6e/0x76
> >
> > [  227.820182] kfence-#0: 0x000000000fac1d5d-0x00000000a28a73c3, size=4096, cache=kmalloc-4k
> >
> > [  227.820185] allocated by task 2559 on cpu 3 at 188.703118s:
> > [  227.820233]  __kmem_cache_alloc_node+0x3c9/0x410
> > [  227.820236]  kmalloc_trace+0x2a/0xa0
> > [  227.820238]  nvme_get_effects_log+0x68/0xd0 [nvme_core]
> > [  227.820251]  nvme_init_identify+0x5ef/0x640 [nvme_core]
> > [  227.820263]  nvme_init_ctrl_finish+0x8d/0x250 [nvme_core]
> > [  227.820275]  nvme_tcp_setup_ctrl+0x1ce/0x710 [nvme_tcp]
> > [  227.820281]  nvme_tcp_create_ctrl+0x359/0x450 [nvme_tcp]
> > [  227.820286]  nvmf_dev_write+0x203/0x3b0 [nvme_fabrics]
> > [  227.820292]  vfs_write+0xd2/0x3d0
> > [  227.820294]  ksys_write+0x5d/0xd0
> > [  227.820297]  do_syscall_64+0x45/0xf0
> > [  227.820298]  entry_SYSCALL_64_after_hwframe+0x6e/0x76
> >
> > [  227.820302] freed by task 2521 on cpu 28 at 220.115945s:
> > [  227.820329]  nvme_free_ctrl+0xa6/0x260 [nvme_core]
> > [  227.820342]  device_release+0x37/0x90
> > [  227.820345]  kobject_release+0x57/0x120
> > [  227.820347]  nvme_sysfs_delete+0x39/0x50 [nvme_core]
> > [  227.820360]  kernfs_fop_write_iter+0x144/0x1e0
> > [  227.820362]  vfs_write+0x25f/0x3d0
> > [  227.820364]  ksys_write+0x5d/0xd0
> > [  227.820366]  do_syscall_64+0x45/0xf0
> > [  227.820368]  entry_SYSCALL_64_after_hwframe+0x6e/0x76
> >
> > Fix this by removing head->effects. Use the Commands Supported and Effects log
> > in ctrl->cels directly.
> >
> > Fixes: be93e87e7802 ("nvme: support for multiple Command Sets Supported and Effects log pages")
> > Signed-off-by: Yuanyuan Zhong <yzhong at purestorage.com>
> > Reviewed-by: Randy Jennings <randyj at purestorage.com>
> > Reviewed-by: Hamilton Coutinho <hcoutinho at purestorage.com>
> > ---
> >  drivers/nvme/host/core.c | 17 ++++++++---------
> >  drivers/nvme/host/nvme.h |  1 -
> >  2 files changed, 8 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> > index 88b54cdcbd68..14fdc2de3a75 100644
> > --- a/drivers/nvme/host/core.c
> > +++ b/drivers/nvme/host/core.c
> > @@ -1085,7 +1085,9 @@ u32 nvme_command_effects(struct nvme_ctrl *ctrl, struct nvme_ns *ns, u8 opcode)
> >       u32 effects = 0;
> >
> >       if (ns) {
> > -             effects = le32_to_cpu(ns->head->effects->iocs[opcode]);
> > +             struct nvme_effects_log *cel = xa_load(&ctrl->cels, ns->head->ids.csi);
> > +
> > +             effects = le32_to_cpu(cel->iocs[opcode]);
> >               if (effects & ~(NVME_CMD_EFFECTS_CSUPP | NVME_CMD_EFFECTS_LBCC))
> >                       dev_warn_once(ctrl->device,
> >                               "IO command:%02x has unusual effects:%08x\n",
>
> This is in the hot path for passthrough, can't we simply reload
> ->effects when we need?

Do you mean something like this? If not, can you please elaborate
"when we need"?
-               struct nvme_effects_log *cel = xa_load(&ctrl->cels,
ns->head->ids.csi);
+               struct nvme_effects_log *cel = (ns->head->ids.csi ==
NVME_CSI_NVM) ?
+                       ctrl->effects : xa_load(&ctrl->cels, ns->head->ids.csi);
Will it be good to change ctrl->effects to ctrl->effects[3] for
already defined CSI?


>
> --
> Jens Axboe
>


--
Regards,
Yuanyuan Zhong