RE: [PATCH v4 3/5] nvme: refactor nvme_submit_user_cmd()

From: Clay Mayers <Clay.Mayers@kioxia.com>
To: Jens Axboe <axboe@kernel.dk>, Kanchan Joshi <joshi.k@samsung.com>,
	"hch@lst.de" <hch@lst.de>
Cc: "io-uring@vger.kernel.org" <io-uring@vger.kernel.org>,
	"linux-nvme@lists.infradead.org" <linux-nvme@lists.infradead.org>,
	"asml.silence@gmail.com" <asml.silence@gmail.com>,
	"ming.lei@redhat.com" <ming.lei@redhat.com>,
	"mcgrof@kernel.org" <mcgrof@kernel.org>,
	"shr@fb.com" <shr@fb.com>,
	"joshiiitr@gmail.com" <joshiiitr@gmail.com>,
	"anuj20.g@samsung.com" <anuj20.g@samsung.com>,
	"gost.dev@samsung.com" <gost.dev@samsung.com>
Subject: RE: [PATCH v4 3/5] nvme: refactor nvme_submit_user_cmd()
Date: Thu, 5 May 2022 19:30:03 +0000	[thread overview]
Message-ID: <0b16682a30434d9c820a888ae0dc9ac5@kioxia.com> (raw)
In-Reply-To: <93e697b1-42c5-d2f4-8fb8-7b5d1892e871@kernel.dk>

On 5/5/22 12:11 PM, Jens Axboe wrote:
> On 5/5/22 1:03 PM, Jens Axboe wrote:
> > On 5/5/22 12:37 PM, Clay Mayers wrote:
> >>> From: Kanchan Joshi
> >>> Sent: Wednesday, May 4, 2022 11:06 PM
> >>> ---
> >>
> >>>  drivers/nvme/host/ioctl.c | 47 ++++++++++++++++++++++++++++++++++---
> --
> >>>  1 file changed, 42 insertions(+), 5 deletions(-)
> >>>
> >>> +static int nvme_execute_user_rq(struct request *req, void __user
> >>> *meta_buffer,
> >>> +		unsigned meta_len, u64 *result)
> >>> +{
> >>> +	struct bio *bio = req->bio;
> >>> +	bool write = bio_op(bio) == REQ_OP_DRV_OUT;
> >>
> >> I'm getting a NULL ptr access on the first
> ioctl(NVME_IOCTL_ADMIN64_CMD)
> >> I send - it has no ubuffer so I think there's no req->bio.
> >
> > Does this work?

It did not!  Same null ptr deref at nearly if not the same location.
I didn't investigate to see the line of code since you had sent v2.

> 
> This might be better, though you'd only notice if you had integrity
> enabled. Christoph, I'm folding this in with patch 3...
> 
> 
> diff --git a/drivers/nvme/host/ioctl.c b/drivers/nvme/host/ioctl.c
> index 8fe7ad18a709..3d827789b536 100644
> --- a/drivers/nvme/host/ioctl.c
> +++ b/drivers/nvme/host/ioctl.c
> @@ -21,9 +21,13 @@ static void __user *nvme_to_user_ptr(uintptr_t ptrval)
> 
>  static inline void *nvme_meta_from_bio(struct bio *bio)
>  {
> -	struct bio_integrity_payload *bip = bio_integrity(bio);
> +	if (bio) {
> +		struct bio_integrity_payload *bip = bio_integrity(bio);
> 
> -	return bip ? bvec_virt(bip->bip_vec) : NULL;
> +		return bip ? bvec_virt(bip->bip_vec) : NULL;
> +	}
> +
> +	return NULL;
>  }
> 
>  /*
> @@ -205,19 +209,20 @@ static int nvme_execute_user_rq(struct request *req,
> void __user *meta_buffer,
>  		unsigned meta_len, u64 *result)
>  {
>  	struct bio *bio = req->bio;
> -	bool write = bio_op(bio) == REQ_OP_DRV_OUT;
> -	int ret;
>  	void *meta = nvme_meta_from_bio(bio);
> +	int ret;
> 
>  	ret = nvme_execute_passthru_rq(req);
> 
>  	if (result)
>  		*result = le64_to_cpu(nvme_req(req)->result.u64);
> -	if (meta && !ret && !write) {
> -		if (copy_to_user(meta_buffer, meta, meta_len))
> +	if (meta) {
> +		bool write = bio_op(bio) == REQ_OP_DRV_OUT;
> +
> +		if (!ret && !write && copy_to_user(meta_buffer, meta,
> meta_len))
>  			ret = -EFAULT;
> +		kfree(meta);
>  	}
> -	kfree(meta);
>  	if (bio)
>  		blk_rq_unmap_user(bio);
>  	blk_mq_free_request(req);
> 
> --
> Jens Axboe

This does work and got me past the null ptr segfault.

Clay.