Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.38 as permitted sender) client-ip=23.128.96.38;
Message-ID: <69218380-45f0-41cc-8a65-50878d44219e@kernel.dk>
Date:   Wed, 15 Nov 2023 12:02:54 -0700
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] nvme-core: remove head->effects to fix use-after-free
Content-Language: en-US
To:     Yuanyuan Zhong <yzhong@purestorage.com>, kbusch@kernel.org,
        hch@lst.de, sagi@grimberg.me
Cc:     linux-nvme@lists.infradead.org, linux-kernel@vger.kernel.org,
        randyj@purestorage.com, hcoutinho@purestorage.com
References: <20231115185439.2616073-1-yzhong@purestorage.com>
From:   Jens Axboe <axboe@kernel.dk>
In-Reply-To: <20231115185439.2616073-1-yzhong@purestorage.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

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@purestorage.com>
> Reviewed-by: Randy Jennings <randyj@purestorage.com>
> Reviewed-by: Hamilton Coutinho <hcoutinho@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?

-- 
Jens Axboe