diff mbox

block: nvme: correct the nvme queue id check

Message ID 1477138157-22337-1-git-send-email-ppandit@redhat.com (mailing list archive)
State New, archived
Headers show

Commit Message

Prasad Pandit Oct. 22, 2016, 12:09 p.m. UTC 
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(-)

Comments

Peter Maydell Oct. 22, 2016, 1:07 p.m. UTC | #1 
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
Prasad Pandit Oct. 23, 2016, 5:08 a.m. UTC | #2 
+-- On Sat, 22 Oct 2016, Peter Maydell wrote --+
| 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?

   nvme_check_sqid(NvmeCtrl *n, uint16_t sqid)
   {
       return sqid < n->num_queues && n->sq[sqid] != NULL ? 0 : -1;
   }

Both 'nvme_check_sqid' and 'nvme_check_cqid' return zero(0) when the 
respective queue id's are valid and -1 when they are invalid. The '!' was 
causing it to return invalid QID error when the check function returned zero.

The code seems incosistent quite a bit. The current check returns invalid QID 
error for '!cqid' and '!sqid', but these IDs are used as index into 'n->cq' 
array, not sure why index zero(0) is invalid. But then I found this in the 
NVME specification[0]

   "...One entry in each queue is not available for use due 
     to Head and Tail entry pointer definition."

Not sure if this one entry is at index zero(0). If so, there are other calls 
to the 'nvme_check' functions which are not acompanyed with '!cqid' or 
'!sqid' checks. Ex.

    nvme_process_db
        qid = (addr - 0x1000) >> 3;
        if (nvme_check_sqid(n, qid)) {
            return;
        }

    nvme_del_sq
        if (!nvme_check_cqid(n, sq->cqid)) {
            cq = n->cq[sq->cqid];

I haven't check these in further details yet. When 'cqid' is invalid it 
returns 'NVME_INVALID_CQID' but when 'sqid' is invalid it returns 
'NVME_INVALID_QID'. Not sure why not use 'NVME_INVALID_QID' for both.

[0] http://www.nvmexpress.org/wp-content/uploads/NVM-Express-1_2-Gold-20141209.pdf

Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F
diff mbox

Patch

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)) {