Re: [Qemu-devel] [PATCH] block: nvme: correct the nvme queue id check

[Peter Maydell <peter.maydell@linaro.org>]

On 22 October 2016 at 13:09, P J P <ppandit@redhat.com> wrote:
> From: Prasad J Pandit <pjp@fedoraproject.org>
>
> 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 <luodalongde@gmail.com>
> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
> ---
>  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