[PATCH] nvme: Fix discard buffer overrun

[PATCH] nvme: Fix discard buffer overrun

[Keith Busch]

This patch checks the discard range array bounds before setting it in
case the driver gets a badly formed request.

Signed-off-by: Keith Busch <keith.busch at intel.com>
Cc: Jens Axboe <axboe at kernel.dk>
---
 drivers/nvme/host/core.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 4bfb4ba6cd14..d02906a6dabe 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -518,9 +518,11 @@ static blk_status_t nvme_setup_discard(struct nvme_ns *ns, struct request *req,
 		u64 slba = nvme_block_nr(ns, bio->bi_iter.bi_sector);
 		u32 nlb = bio->bi_iter.bi_size >> ns->lba_shift;
 
-		range[n].cattr = cpu_to_le32(0);
-		range[n].nlb = cpu_to_le32(nlb);
-		range[n].slba = cpu_to_le64(slba);
+		if (n < segments) {
+			range[n].cattr = cpu_to_le32(0);
+			range[n].nlb = cpu_to_le32(nlb);
+			range[n].slba = cpu_to_le64(slba);
+		}
 		n++;
 	}
 
-- 
2.14.3

[PATCH] nvme: Fix discard buffer overrun

[jianchao.wang]

Hi Keith

Since each discard bio merged into a request is counted as one segment,
why not let blk_rq_nr_discard_segments return numbers of bios directly.

On 02/01/2018 08:01 AM, Keith Busch wrote:
> This patch checks the discard range array bounds before setting it in
> case the driver gets a badly formed request.
> 
> Signed-off-by: Keith Busch <keith.busch at intel.com>
> Cc: Jens Axboe <axboe at kernel.dk>
> ---
>  drivers/nvme/host/core.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 4bfb4ba6cd14..d02906a6dabe 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -518,9 +518,11 @@ static blk_status_t nvme_setup_discard(struct nvme_ns *ns, struct request *req,
>  		u64 slba = nvme_block_nr(ns, bio->bi_iter.bi_sector);
>  		u32 nlb = bio->bi_iter.bi_size >> ns->lba_shift;
>  
> -		range[n].cattr = cpu_to_le32(0);
> -		range[n].nlb = cpu_to_le32(nlb);
> -		range[n].slba = cpu_to_le64(slba);
> +		if (n < segments) {
> +			range[n].cattr = cpu_to_le32(0);
> +			range[n].nlb = cpu_to_le32(nlb);
> +			range[n].slba = cpu_to_le64(slba);
> +		}
>  		n++;
>  	}
 

[PATCH] nvme: Fix discard buffer overrun

[Keith Busch]

On Thu, Feb 01, 2018@10:55:30PM +0800, jianchao.wang wrote:
> Hi Keith
> 
> Since each discard bio merged into a request is counted as one segment,
> why not let blk_rq_nr_discard_segments return numbers of bios directly.

Right, I'm all for fixing the discard segment count. At the same time,
I don't want to depend on it's correctness to not access out-of-bounds
array indices.

[PATCH] nvme: Fix discard buffer overrun

[jianchao.wang]

Hi Keith

Thanks for your kindly response.

On 02/01/2018 11:05 PM, Keith Busch wrote:
> On Thu, Feb 01, 2018@10:55:30PM +0800, jianchao.wang wrote:
>> Hi Keith
>>
>> Since each discard bio merged into a request is counted as one segment,
>> why not let blk_rq_nr_discard_segments return numbers of bios directly.
> 
> Right, I'm all for fixing the discard segment count. At the same time,
> I don't want to depend on it's correctness to not access out-of-bounds
> array indices.
> 
Actually, if we use iterating request bio list to return discard segments count,
we won't have any out-of-bounds error any more. And things will be simpler. :)

Thanks
Jianchao

[PATCH] nvme: Fix discard buffer overrun

[Jens Axboe]

On 2/1/18 7:55 AM, jianchao.wang wrote:
> Hi Keith
> 
> Since each discard bio merged into a request is counted as one segment,
> why not let blk_rq_nr_discard_segments return numbers of bios directly.

That's a good idea, but for now this looks fine. Both this and the
core block hole can be fixed, and then someone can hopefully work
on making discards behave a bit more nicely after that.

-- 
Jens Axboe

[PATCH] nvme: Fix discard buffer overrun

[jianchao.wang]

Hi Jens

Thanks for your kindly response.

On 02/01/2018 11:27 PM, Jens Axboe wrote:
> On 2/1/18 7:55 AM, jianchao.wang wrote:
>> Hi Keith
>>
>> Since each discard bio merged into a request is counted as one segment,
>> why not let blk_rq_nr_discard_segments return numbers of bios directly.
> 
> That's a good idea, but for now this looks fine. Both this and the
> core block hole can be fixed, and then someone can hopefully work
> on making discards behave a bit more nicely after that.
> 
Yes, got it.

Thanks
Jianchao

[PATCH] nvme: Fix discard buffer overrun

[Jens Axboe]

On 1/31/18 5:01 PM, Keith Busch wrote:
> This patch checks the discard range array bounds before setting it in
> case the driver gets a badly formed request.
> 
> Signed-off-by: Keith Busch <keith.busch at intel.com>
> Cc: Jens Axboe <axboe at kernel.dk>
> ---
>  drivers/nvme/host/core.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 4bfb4ba6cd14..d02906a6dabe 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -518,9 +518,11 @@ static blk_status_t nvme_setup_discard(struct nvme_ns *ns, struct request *req,
>  		u64 slba = nvme_block_nr(ns, bio->bi_iter.bi_sector);
>  		u32 nlb = bio->bi_iter.bi_size >> ns->lba_shift;
>  
> -		range[n].cattr = cpu_to_le32(0);
> -		range[n].nlb = cpu_to_le32(nlb);
> -		range[n].slba = cpu_to_le64(slba);
> +		if (n < segments) {
> +			range[n].cattr = cpu_to_le32(0);
> +			range[n].nlb = cpu_to_le32(nlb);
> +			range[n].slba = cpu_to_le64(slba);
> +		}
>  		n++;
>  	}

Reviewed-by: Jens Axboe <axboe at kernel.dk>

-- 
Jens Axboe