diff mbox series

[2/9] hw/block/nvme: fix log page offset check

Message ID 20200930220414.562527-3-kbusch@kernel.org
State New
Headers show
Series nvme qemu cleanups and fixes | expand

Commit Message

Keith Busch Sept. 30, 2020, 10:04 p.m. UTC 
Return error if the requested offset starts after the size of the log
being returned. Also, move the check for earlier in the function so
we're not doing unnecessary calculations.

Signed-off-by: Keith Busch <kbusch@kernel.org>
---
 hw/block/nvme.c | 22 ++++++++++------------
 1 file changed, 10 insertions(+), 12 deletions(-)

Comments

Dmitry Fomichev Sept. 30, 2020, 11:18 p.m. UTC | #1 
> -----Original Message-----
> From: Keith Busch <kbusch@kernel.org>
> Sent: Wednesday, September 30, 2020 6:04 PM
> To: qemu-block@nongnu.org; qemu-devel@nongnu.org; Klaus Jensen
> <k.jensen@samsung.com>
> Cc: Niklas Cassel <Niklas.Cassel@wdc.com>; Dmitry Fomichev
> <Dmitry.Fomichev@wdc.com>; Kevin Wolf <kwolf@redhat.com>; Philippe
> Mathieu-Daudé <philmd@redhat.com>; Keith Busch <kbusch@kernel.org>
> Subject: [PATCH 2/9] hw/block/nvme: fix log page offset check
> 
> Return error if the requested offset starts after the size of the log
> being returned. Also, move the check for earlier in the function so
> we're not doing unnecessary calculations.
> 
> Signed-off-by: Keith Busch <kbusch@kernel.org>

Reviewed- by: Dmitry Fomichev <dmitry.fomichev@wdc.com>

> ---
>  hw/block/nvme.c | 22 ++++++++++------------
>  1 file changed, 10 insertions(+), 12 deletions(-)
> 
> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> index db52ea0db9..8d2b5be567 100644
> --- a/hw/block/nvme.c
> +++ b/hw/block/nvme.c
> @@ -1179,6 +1179,10 @@ static uint16_t nvme_smart_info(NvmeCtrl *n,
> uint8_t rae, uint32_t buf_len,
>          return NVME_INVALID_FIELD | NVME_DNR;
>      }
> 
> +    if (off >= sizeof(smart)) {
> +        return NVME_INVALID_FIELD | NVME_DNR;
> +    }
> +
>      for (int i = 1; i <= n->num_namespaces; i++) {
>          NvmeNamespace *ns = nvme_ns(n, i);
>          if (!ns) {
> @@ -1193,10 +1197,6 @@ static uint16_t nvme_smart_info(NvmeCtrl *n,
> uint8_t rae, uint32_t buf_len,
>          write_commands += s->nr_ops[BLOCK_ACCT_WRITE];
>      }
> 
> -    if (off > sizeof(smart)) {
> -        return NVME_INVALID_FIELD | NVME_DNR;
> -    }
> -
>      trans_len = MIN(sizeof(smart) - off, buf_len);
> 
>      memset(&smart, 0x0, sizeof(smart));
> @@ -1234,12 +1234,11 @@ static uint16_t nvme_fw_log_info(NvmeCtrl *n,
> uint32_t buf_len, uint64_t off,
>          .afi = 0x1,
>      };
> 
> -    strpadcpy((char *)&fw_log.frs1, sizeof(fw_log.frs1), "1.0", ' ');
> -
> -    if (off > sizeof(fw_log)) {
> +    if (off >= sizeof(fw_log)) {
>          return NVME_INVALID_FIELD | NVME_DNR;
>      }
> 
> +    strpadcpy((char *)&fw_log.frs1, sizeof(fw_log.frs1), "1.0", ' ');
>      trans_len = MIN(sizeof(fw_log) - off, buf_len);
> 
>      return nvme_dma(n, (uint8_t *) &fw_log + off, trans_len,
> @@ -1252,16 +1251,15 @@ static uint16_t nvme_error_info(NvmeCtrl *n,
> uint8_t rae, uint32_t buf_len,
>      uint32_t trans_len;
>      NvmeErrorLog errlog;
> 
> -    if (!rae) {
> -        nvme_clear_events(n, NVME_AER_TYPE_ERROR);
> +    if (off >= sizeof(errlog)) {
> +        return NVME_INVALID_FIELD | NVME_DNR;
>      }
> 
> -    if (off > sizeof(errlog)) {
> -        return NVME_INVALID_FIELD | NVME_DNR;
> +    if (!rae) {
> +        nvme_clear_events(n, NVME_AER_TYPE_ERROR);
>      }
> 
>      memset(&errlog, 0x0, sizeof(errlog));
> -
>      trans_len = MIN(sizeof(errlog) - off, buf_len);
> 
>      return nvme_dma(n, (uint8_t *)&errlog, trans_len,
> --
> 2.24.1
Klaus Jensen Oct. 1, 2020, 4:05 a.m. UTC | #2 
On Sep 30 15:04, Keith Busch wrote:
> Return error if the requested offset starts after the size of the log
> being returned. Also, move the check for earlier in the function so
> we're not doing unnecessary calculations.
> 
> Signed-off-by: Keith Busch <kbusch@kernel.org>

Reviewed-by: Klaus Jensen <k.jensen@samsung.com>

> ---
>  hw/block/nvme.c | 22 ++++++++++------------
>  1 file changed, 10 insertions(+), 12 deletions(-)
> 
> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> index db52ea0db9..8d2b5be567 100644
> --- a/hw/block/nvme.c
> +++ b/hw/block/nvme.c
> @@ -1179,6 +1179,10 @@ static uint16_t nvme_smart_info(NvmeCtrl *n, uint8_t rae, uint32_t buf_len,
>          return NVME_INVALID_FIELD | NVME_DNR;
>      }
>  
> +    if (off >= sizeof(smart)) {
> +        return NVME_INVALID_FIELD | NVME_DNR;
> +    }
> +
>      for (int i = 1; i <= n->num_namespaces; i++) {
>          NvmeNamespace *ns = nvme_ns(n, i);
>          if (!ns) {
> @@ -1193,10 +1197,6 @@ static uint16_t nvme_smart_info(NvmeCtrl *n, uint8_t rae, uint32_t buf_len,
>          write_commands += s->nr_ops[BLOCK_ACCT_WRITE];
>      }
>  
> -    if (off > sizeof(smart)) {
> -        return NVME_INVALID_FIELD | NVME_DNR;
> -    }
> -
>      trans_len = MIN(sizeof(smart) - off, buf_len);
>  
>      memset(&smart, 0x0, sizeof(smart));
> @@ -1234,12 +1234,11 @@ static uint16_t nvme_fw_log_info(NvmeCtrl *n, uint32_t buf_len, uint64_t off,
>          .afi = 0x1,
>      };
>  
> -    strpadcpy((char *)&fw_log.frs1, sizeof(fw_log.frs1), "1.0", ' ');
> -
> -    if (off > sizeof(fw_log)) {
> +    if (off >= sizeof(fw_log)) {
>          return NVME_INVALID_FIELD | NVME_DNR;
>      }
>  
> +    strpadcpy((char *)&fw_log.frs1, sizeof(fw_log.frs1), "1.0", ' ');
>      trans_len = MIN(sizeof(fw_log) - off, buf_len);
>  
>      return nvme_dma(n, (uint8_t *) &fw_log + off, trans_len,
> @@ -1252,16 +1251,15 @@ static uint16_t nvme_error_info(NvmeCtrl *n, uint8_t rae, uint32_t buf_len,
>      uint32_t trans_len;
>      NvmeErrorLog errlog;
>  
> -    if (!rae) {
> -        nvme_clear_events(n, NVME_AER_TYPE_ERROR);
> +    if (off >= sizeof(errlog)) {
> +        return NVME_INVALID_FIELD | NVME_DNR;
>      }
>  
> -    if (off > sizeof(errlog)) {
> -        return NVME_INVALID_FIELD | NVME_DNR;
> +    if (!rae) {
> +        nvme_clear_events(n, NVME_AER_TYPE_ERROR);
>      }
>  
>      memset(&errlog, 0x0, sizeof(errlog));
> -
>      trans_len = MIN(sizeof(errlog) - off, buf_len);
>  
>      return nvme_dma(n, (uint8_t *)&errlog, trans_len,
> -- 
> 2.24.1
> 
>
Philippe Mathieu-Daudé Oct. 1, 2020, 10:11 a.m. UTC | #3 
On 10/1/20 12:04 AM, Keith Busch wrote:
> Return error if the requested offset starts after the size of the log
> being returned. Also, move the check for earlier in the function so
> we're not doing unnecessary calculations.
> 
> Signed-off-by: Keith Busch <kbusch@kernel.org>
> ---
>  hw/block/nvme.c | 22 ++++++++++------------
>  1 file changed, 10 insertions(+), 12 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
diff mbox series

Patch

diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index db52ea0db9..8d2b5be567 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -1179,6 +1179,10 @@  static uint16_t nvme_smart_info(NvmeCtrl *n, uint8_t rae, uint32_t buf_len,
         return NVME_INVALID_FIELD | NVME_DNR;
     }
 
+    if (off >= sizeof(smart)) {
+        return NVME_INVALID_FIELD | NVME_DNR;
+    }
+
     for (int i = 1; i <= n->num_namespaces; i++) {
         NvmeNamespace *ns = nvme_ns(n, i);
         if (!ns) {
@@ -1193,10 +1197,6 @@  static uint16_t nvme_smart_info(NvmeCtrl *n, uint8_t rae, uint32_t buf_len,
         write_commands += s->nr_ops[BLOCK_ACCT_WRITE];
     }
 
-    if (off > sizeof(smart)) {
-        return NVME_INVALID_FIELD | NVME_DNR;
-    }
-
     trans_len = MIN(sizeof(smart) - off, buf_len);
 
     memset(&smart, 0x0, sizeof(smart));
@@ -1234,12 +1234,11 @@  static uint16_t nvme_fw_log_info(NvmeCtrl *n, uint32_t buf_len, uint64_t off,
         .afi = 0x1,
     };
 
-    strpadcpy((char *)&fw_log.frs1, sizeof(fw_log.frs1), "1.0", ' ');
-
-    if (off > sizeof(fw_log)) {
+    if (off >= sizeof(fw_log)) {
         return NVME_INVALID_FIELD | NVME_DNR;
     }
 
+    strpadcpy((char *)&fw_log.frs1, sizeof(fw_log.frs1), "1.0", ' ');
     trans_len = MIN(sizeof(fw_log) - off, buf_len);
 
     return nvme_dma(n, (uint8_t *) &fw_log + off, trans_len,
@@ -1252,16 +1251,15 @@  static uint16_t nvme_error_info(NvmeCtrl *n, uint8_t rae, uint32_t buf_len,
     uint32_t trans_len;
     NvmeErrorLog errlog;
 
-    if (!rae) {
-        nvme_clear_events(n, NVME_AER_TYPE_ERROR);
+    if (off >= sizeof(errlog)) {
+        return NVME_INVALID_FIELD | NVME_DNR;
     }
 
-    if (off > sizeof(errlog)) {
-        return NVME_INVALID_FIELD | NVME_DNR;
+    if (!rae) {
+        nvme_clear_events(n, NVME_AER_TYPE_ERROR);
     }
 
     memset(&errlog, 0x0, sizeof(errlog));
-
     trans_len = MIN(sizeof(errlog) - off, buf_len);
 
     return nvme_dma(n, (uint8_t *)&errlog, trans_len,