[PATCH 1/2] block/file-posix: fix g_file_get_contents return path

[PATCH 1/2] block/file-posix: fix g_file_get_contents return path

[Sam Li]

The g_file_get_contents() function returns a g_boolean. If it fails, the
returned value will be 0 instead of -1. Solve the issue by skipping
assigning ret value.

This issue was found by Matthew Rosato using virtio-blk-{pci,ccw} backed
by an NVMe partition e.g. /dev/nvme0n1p1 on s390x.

Signed-off-by: Sam Li <faithilikerun@gmail.com>
---
 block/file-posix.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/block/file-posix.c b/block/file-posix.c
index ac1ed54811..0d9d179a35 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -1232,7 +1232,6 @@ static int hdev_get_max_hw_transfer(int fd, struct stat *st)
 static int get_sysfs_str_val(struct stat *st, const char *attribute,
                              char **val) {
     g_autofree char *sysfspath = NULL;
-    int ret;
     size_t len;
 
     if (!S_ISBLK(st->st_mode)) {
@@ -1242,8 +1241,7 @@ static int get_sysfs_str_val(struct stat *st, const char *attribute,
     sysfspath = g_strdup_printf("/sys/dev/block/%u:%u/queue/%s",
                                 major(st->st_rdev), minor(st->st_rdev),
                                 attribute);
-    ret = g_file_get_contents(sysfspath, val, &len, NULL);
-    if (ret == -1) {
+    if (!g_file_get_contents(sysfspath, val, &len, NULL)) {
         return -ENOENT;
     }
 
@@ -1253,7 +1251,7 @@ static int get_sysfs_str_val(struct stat *st, const char *attribute,
     if (*(p + len - 1) == '\n') {
         *(p + len - 1) = '\0';
     }
-    return ret;
+    return 0;
 }
 #endif
 
-- 
2.40.1

[PATCH 2/2] block/file-posix: fix wps checking in raw_co_prw

[Sam Li]

If the write operation fails and the wps is NULL, then accessing it will
lead to data corruption.

Solving the issue by adding a nullptr checking in get_zones_wp() where
the wps is used.

This issue is found by Peter Maydell using the Coverity Tool (CID
1512459).

Signed-off-by: Sam Li <faithilikerun@gmail.com>
---
 block/file-posix.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/block/file-posix.c b/block/file-posix.c
index 0d9d179a35..620942bf40 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -1340,6 +1340,10 @@ static int get_zones_wp(BlockDriverState *bs, int fd, int64_t offset,
     rep_size = sizeof(struct blk_zone_report) + nrz * sizeof(struct blk_zone);
     g_autofree struct blk_zone_report *rep = NULL;
 
+    if (!wps) {
+        return -1;
+    }
+
     rep = g_malloc(rep_size);
     blkz = (struct blk_zone *)(rep + 1);
     while (n < nrz) {
-- 
2.40.1

Re: [PATCH 2/2] block/file-posix: fix wps checking in raw_co_prw

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 2292 bytes --]

On Sun, Jun 04, 2023 at 02:16:58PM +0800, Sam Li wrote:
> If the write operation fails and the wps is NULL, then accessing it will
> lead to data corruption.
> 
> Solving the issue by adding a nullptr checking in get_zones_wp() where
> the wps is used.
> 
> This issue is found by Peter Maydell using the Coverity Tool (CID
> 1512459).
> 
> Signed-off-by: Sam Li <faithilikerun@gmail.com>
> ---
>  block/file-posix.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/block/file-posix.c b/block/file-posix.c
> index 0d9d179a35..620942bf40 100644
> --- a/block/file-posix.c
> +++ b/block/file-posix.c
> @@ -1340,6 +1340,10 @@ static int get_zones_wp(BlockDriverState *bs, int fd, int64_t offset,
>      rep_size = sizeof(struct blk_zone_report) + nrz * sizeof(struct blk_zone);
>      g_autofree struct blk_zone_report *rep = NULL;
>  
> +    if (!wps) {
> +        return -1;
> +    }

An error will be printed every time this happens on a non-zoned device:

  static void update_zones_wp(BlockDriverState *bs, int fd, int64_t offset,
                              unsigned int nrz)
  {
      if (get_zones_wp(bs, fd, offset, nrz, 0) < 0) {
          error_report("update zone wp failed");

Please change the following code to avoid the call to update_zones_wp():

  #if defined(CONFIG_BLKZONED)
  {
      BlockZoneWps *wps = bs->wps;
      if (ret == 0) {
          if ((type & (QEMU_AIO_WRITE | QEMU_AIO_ZONE_APPEND))
              && wps && bs->bl.zone_size) {
              uint64_t *wp = &wps->wp[offset / bs->bl.zone_size];
              if (!BDRV_ZT_IS_CONV(*wp)) {
                  if (type & QEMU_AIO_ZONE_APPEND) {
                      *s->offset = *wp;
                      trace_zbd_zone_append_complete(bs, *s->offset
                          >> BDRV_SECTOR_BITS);
                  }
                  /* Advance the wp if needed */
                  if (offset + bytes > *wp) {
                      *wp = offset + bytes;
                  }
              }
          }
      } else {
-         if (type & (QEMU_AIO_WRITE | QEMU_AIO_ZONE_APPEND)) {
+         if (wps && (type & (QEMU_AIO_WRITE | QEMU_AIO_ZONE_APPEND))) {
              update_zones_wp(bs, s->fd, 0, 1);
          }
      }

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/2] block/file-posix: fix g_file_get_contents return path

[Matthew Rosato]

On 6/4/23 2:16 AM, Sam Li wrote:
> The g_file_get_contents() function returns a g_boolean. If it fails, the
> returned value will be 0 instead of -1. Solve the issue by skipping
> assigning ret value.
> 
> This issue was found by Matthew Rosato using virtio-blk-{pci,ccw} backed
> by an NVMe partition e.g. /dev/nvme0n1p1 on s390x.
> 
> Signed-off-by: Sam Li <faithilikerun@gmail.com>

Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>

Also tested to verify that this fix resolves the reported issue.  Thanks!

Re: [PATCH 1/2] block/file-posix: fix g_file_get_contents return path

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 666 bytes --]

On Sun, Jun 04, 2023 at 02:16:57PM +0800, Sam Li wrote:
> The g_file_get_contents() function returns a g_boolean. If it fails, the
> returned value will be 0 instead of -1. Solve the issue by skipping
> assigning ret value.
> 
> This issue was found by Matthew Rosato using virtio-blk-{pci,ccw} backed
> by an NVMe partition e.g. /dev/nvme0n1p1 on s390x.
> 
> Signed-off-by: Sam Li <faithilikerun@gmail.com>
> ---
>  block/file-posix.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)

The number of bytes returned was never used, so changing the return
value to 0 or -errno is fine:

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/2] block/file-posix: fix g_file_get_contents return path

[Matthew Rosato]

On 6/4/23 2:16 AM, Sam Li wrote:
> The g_file_get_contents() function returns a g_boolean. If it fails, the
> returned value will be 0 instead of -1. Solve the issue by skipping
> assigning ret value.
> 
> This issue was found by Matthew Rosato using virtio-blk-{pci,ccw} backed
> by an NVMe partition e.g. /dev/nvme0n1p1 on s390x.
> 
> Signed-off-by: Sam Li <faithilikerun@gmail.com>

Polite ping on this patch -- this issue still exists in master as of today and this patch resolves it for me.  Just want to make sure it gets into 8.1

Thanks,
Matt

> ---
>  block/file-posix.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/block/file-posix.c b/block/file-posix.c
> index ac1ed54811..0d9d179a35 100644
> --- a/block/file-posix.c
> +++ b/block/file-posix.c
> @@ -1232,7 +1232,6 @@ static int hdev_get_max_hw_transfer(int fd, struct stat *st)
>  static int get_sysfs_str_val(struct stat *st, const char *attribute,
>                               char **val) {
>      g_autofree char *sysfspath = NULL;
> -    int ret;
>      size_t len;
>  
>      if (!S_ISBLK(st->st_mode)) {
> @@ -1242,8 +1241,7 @@ static int get_sysfs_str_val(struct stat *st, const char *attribute,
>      sysfspath = g_strdup_printf("/sys/dev/block/%u:%u/queue/%s",
>                                  major(st->st_rdev), minor(st->st_rdev),
>                                  attribute);
> -    ret = g_file_get_contents(sysfspath, val, &len, NULL);
> -    if (ret == -1) {
> +    if (!g_file_get_contents(sysfspath, val, &len, NULL)) {
>          return -ENOENT;
>      }
>  
> @@ -1253,7 +1251,7 @@ static int get_sysfs_str_val(struct stat *st, const char *attribute,
>      if (*(p + len - 1) == '\n') {
>          *(p + len - 1) = '\0';
>      }
> -    return ret;
> +    return 0;
>  }
>  #endif
>  

Re: [PATCH 1/2] block/file-posix: fix g_file_get_contents return path

[Matthew Rosato]

On 7/5/23 10:54 AM, Matthew Rosato wrote:
> On 6/4/23 2:16 AM, Sam Li wrote:
>> The g_file_get_contents() function returns a g_boolean. If it fails, the
>> returned value will be 0 instead of -1. Solve the issue by skipping
>> assigning ret value.
>>
>> This issue was found by Matthew Rosato using virtio-blk-{pci,ccw} backed
>> by an NVMe partition e.g. /dev/nvme0n1p1 on s390x.
>>
>> Signed-off-by: Sam Li <faithilikerun@gmail.com>
> 
> Polite ping on this patch -- this issue still exists in master as of today and this patch resolves it for me.  Just want to make sure it gets into 8.1
> 

Ping -- I can still reproduce this crash on -rc1.  Any chance this patch can get picked up for the 8.1 release?

@Sam I see you sent a v2 of only patch #2 in this series ('block/file-posix: fix wps checking in raw_co_prw')..  I wonder if this one just got forgotten since it wasn't sent as part of v2.  Maybe try a resend of this patch by itself (plus the review tags added)?

Thanks,
Matt

> 
>> ---
>>  block/file-posix.c | 6 ++----
>>  1 file changed, 2 insertions(+), 4 deletions(-)
>>
>> diff --git a/block/file-posix.c b/block/file-posix.c
>> index ac1ed54811..0d9d179a35 100644
>> --- a/block/file-posix.c
>> +++ b/block/file-posix.c
>> @@ -1232,7 +1232,6 @@ static int hdev_get_max_hw_transfer(int fd, struct stat *st)
>>  static int get_sysfs_str_val(struct stat *st, const char *attribute,
>>                               char **val) {
>>      g_autofree char *sysfspath = NULL;
>> -    int ret;
>>      size_t len;
>>  
>>      if (!S_ISBLK(st->st_mode)) {
>> @@ -1242,8 +1241,7 @@ static int get_sysfs_str_val(struct stat *st, const char *attribute,
>>      sysfspath = g_strdup_printf("/sys/dev/block/%u:%u/queue/%s",
>>                                  major(st->st_rdev), minor(st->st_rdev),
>>                                  attribute);
>> -    ret = g_file_get_contents(sysfspath, val, &len, NULL);
>> -    if (ret == -1) {
>> +    if (!g_file_get_contents(sysfspath, val, &len, NULL)) {
>>          return -ENOENT;
>>      }
>>  
>> @@ -1253,7 +1251,7 @@ static int get_sysfs_str_val(struct stat *st, const char *attribute,
>>      if (*(p + len - 1) == '\n') {
>>          *(p + len - 1) = '\0';
>>      }
>> -    return ret;
>> +    return 0;
>>  }
>>  #endif
>>  
> 
> 

Re: [PATCH 1/2] block/file-posix: fix g_file_get_contents return path

[Sam Li]

Matthew Rosato <mjrosato@linux.ibm.com> 于2023年7月27日周四 19:46写道：
>
> On 7/5/23 10:54 AM, Matthew Rosato wrote:
> > On 6/4/23 2:16 AM, Sam Li wrote:
> >> The g_file_get_contents() function returns a g_boolean. If it fails, the
> >> returned value will be 0 instead of -1. Solve the issue by skipping
> >> assigning ret value.
> >>
> >> This issue was found by Matthew Rosato using virtio-blk-{pci,ccw} backed
> >> by an NVMe partition e.g. /dev/nvme0n1p1 on s390x.
> >>
> >> Signed-off-by: Sam Li <faithilikerun@gmail.com>
> >
> > Polite ping on this patch -- this issue still exists in master as of today and this patch resolves it for me.  Just want to make sure it gets into 8.1
> >
>
> Ping -- I can still reproduce this crash on -rc1.  Any chance this patch can get picked up for the 8.1 release?
>
> @Sam I see you sent a v2 of only patch #2 in this series ('block/file-posix: fix wps checking in raw_co_prw')..  I wonder if this one just got forgotten since it wasn't sent as part of v2.  Maybe try a resend of this patch by itself (plus the review tags added)?

Ok, I will resend it as a separate patch.

>
> Thanks,
> Matt
>
> >
> >> ---
> >>  block/file-posix.c | 6 ++----
> >>  1 file changed, 2 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/block/file-posix.c b/block/file-posix.c
> >> index ac1ed54811..0d9d179a35 100644
> >> --- a/block/file-posix.c
> >> +++ b/block/file-posix.c
> >> @@ -1232,7 +1232,6 @@ static int hdev_get_max_hw_transfer(int fd, struct stat *st)
> >>  static int get_sysfs_str_val(struct stat *st, const char *attribute,
> >>                               char **val) {
> >>      g_autofree char *sysfspath = NULL;
> >> -    int ret;
> >>      size_t len;
> >>
> >>      if (!S_ISBLK(st->st_mode)) {
> >> @@ -1242,8 +1241,7 @@ static int get_sysfs_str_val(struct stat *st, const char *attribute,
> >>      sysfspath = g_strdup_printf("/sys/dev/block/%u:%u/queue/%s",
> >>                                  major(st->st_rdev), minor(st->st_rdev),
> >>                                  attribute);
> >> -    ret = g_file_get_contents(sysfspath, val, &len, NULL);
> >> -    if (ret == -1) {
> >> +    if (!g_file_get_contents(sysfspath, val, &len, NULL)) {
> >>          return -ENOENT;
> >>      }
> >>
> >> @@ -1253,7 +1251,7 @@ static int get_sysfs_str_val(struct stat *st, const char *attribute,
> >>      if (*(p + len - 1) == '\n') {
> >>          *(p + len - 1) = '\0';
> >>      }
> >> -    return ret;
> >> +    return 0;
> >>  }
> >>  #endif
> >>
> >
> >
>