[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] block: nvme: correct the nvme queue id check
From: |
Peter Maydell |
Subject: |
Re: [Qemu-devel] [PATCH] block: nvme: correct the nvme queue id check |
Date: |
Sat, 22 Oct 2016 14:07:49 +0100 |
On 22 October 2016 at 13:09, P J P <address@hidden> wrote:
> From: Prasad J Pandit <address@hidden>
>
> NVME Express Controller has two queues, submission & completion
> queue. When creating a new queue object, 'nvme_create_sq' and
> 'nvme_create_cq' routines incorrectly check the queue id field.
> It could lead to an OOB access issue. Correct the queue id check
> to avoid it.
>
> Reported-by: Qinghao Tang <address@hidden>
> Signed-off-by: Prasad J Pandit <address@hidden>
> ---
> hw/block/nvme.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> index 2ded247..61bdc9d 100644
> --- a/hw/block/nvme.c
> +++ b/hw/block/nvme.c
> @@ -373,7 +373,7 @@ static uint16_t nvme_create_sq(NvmeCtrl *n, NvmeCmd *cmd)
> if (!cqid || nvme_check_cqid(n, cqid)) {
> return NVME_INVALID_CQID | NVME_DNR;
> }
> - if (!sqid || (sqid && !nvme_check_sqid(n, sqid))) {
> + if (!sqid || nvme_check_sqid(n, sqid)) {
> return NVME_INVALID_QID | NVME_DNR;
> }
> if (!qsize || qsize > NVME_CAP_MQES(n->bar.cap)) {
> @@ -447,7 +447,7 @@ static uint16_t nvme_create_cq(NvmeCtrl *n, NvmeCmd *cmd)
> uint16_t qflags = le16_to_cpu(c->cq_flags);
> uint64_t prp1 = le64_to_cpu(c->prp1);
>
> - if (!cqid || (cqid && !nvme_check_cqid(n, cqid))) {
> + if (!cqid || nvme_check_cqid(n, cqid)) {
> return NVME_INVALID_CQID | NVME_DNR;
> }
> if (!qsize || qsize > NVME_CAP_MQES(n->bar.cap)) {
This looks a bit weird. Firstly, it doesn't match
the commit message, because it's still going to
call nvme_check_cqid() and nvme_check_sqid()
in the same situations, so if the commit message is
right and this is going to cause a problem then
the change doesn't seem to be sufficient.
Secondly, it's almost the same as this cleanup
patch from Thomas Huth that's already in qemu-trivial:
http://patchwork.ozlabs.org/patch/681349/
except that your version is removing the !
negations from the return value.
Can you explain in a bit more detail what the bug
is here and why this is the right fix for it?
thanks
-- PMM