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 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/file-posix.c b/block/file-posix.c
index ac1ed54811..4a6c71c7f5 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -2523,7 +2523,7 @@ out:
             }
         }
     } else {
-        if (type & (QEMU_AIO_WRITE | QEMU_AIO_ZONE_APPEND)) {
+        if (type & (QEMU_AIO_WRITE | QEMU_AIO_ZONE_APPEND) && wps) {
             update_zones_wp(bs, s->fd, 0, 1);
         }
     }
-- 
2.40.1

On 6/8/23 03:57, 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 | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/block/file-posix.c b/block/file-posix.c
> index ac1ed54811..4a6c71c7f5 100644
> --- a/block/file-posix.c
> +++ b/block/file-posix.c
> @@ -2523,7 +2523,7 @@ out:
>              }
>          }
>      } else {
> -        if (type & (QEMU_AIO_WRITE | QEMU_AIO_ZONE_APPEND)) {
> +        if (type & (QEMU_AIO_WRITE | QEMU_AIO_ZONE_APPEND) && wps) {
>              update_zones_wp(bs, s->fd, 0, 1);

Nit: this could be:

	} else if (wps && type & (QEMU_AIO_WRITE | QEMU_AIO_ZONE_APPEND)) {

However, both if & else side do something only if the above condition is true
and we only need to that for a zoned drive. So the entire code block could
really be simplified to be a lot more readable. Something like this (totally
untested, not even compiled):

#if defined(CONFIG_BLKZONED)
    if (bs->bl.zone_size && (type & (QEMU_AIO_WRITE | QEMU_AIO_ZONE_APPEND))) {
        BlockZoneWps *wps = bs->wps;
        uint64_t *wp;

        if (!wps) {
            return ret;
        }

        if (ret) {
            /* write error: update the wp from the underlying device */
            update_zones_wp(bs, s->fd, 0, 1);
            goto unlock;
        }

        wp = &wps->wp[offset / bs->bl.zone_size];
        if (BDRV_ZT_IS_CONV(*wp)) {
            /* Conventional zones do not have a write pointer */
            goto unlock;
        }

        /* Return the written position for zone append */
        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;
        }

unlock:
        qemu_co_mutex_unlock(&wps->colock);
    }
#endif

And making this entire block a helper function (e.g. advance_zone_wp()) would
further clean the code. But that should be done in another patch. Care to send one ?

-- 
Damien Le Moal
Western Digital Research

Damien Le Moal <dlemoal@kernel.org> 于2023年6月8日周四 09:29写道：
>
> On 6/8/23 03:57, 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 | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/block/file-posix.c b/block/file-posix.c
> > index ac1ed54811..4a6c71c7f5 100644
> > --- a/block/file-posix.c
> > +++ b/block/file-posix.c
> > @@ -2523,7 +2523,7 @@ out:
> >              }
> >          }
> >      } else {
> > -        if (type & (QEMU_AIO_WRITE | QEMU_AIO_ZONE_APPEND)) {
> > +        if (type & (QEMU_AIO_WRITE | QEMU_AIO_ZONE_APPEND) && wps) {
> >              update_zones_wp(bs, s->fd, 0, 1);
>
> Nit: this could be:
>
>         } else if (wps && type & (QEMU_AIO_WRITE | QEMU_AIO_ZONE_APPEND)) {
>
> However, both if & else side do something only if the above condition is true
> and we only need to that for a zoned drive. So the entire code block could
> really be simplified to be a lot more readable. Something like this (totally
> untested, not even compiled):
>
> #if defined(CONFIG_BLKZONED)
>     if (bs->bl.zone_size && (type & (QEMU_AIO_WRITE | QEMU_AIO_ZONE_APPEND))) {
>         BlockZoneWps *wps = bs->wps;
>         uint64_t *wp;
>
>         if (!wps) {
>             return ret;
>         }
>
>         if (ret) {
>             /* write error: update the wp from the underlying device */
>             update_zones_wp(bs, s->fd, 0, 1);
>             goto unlock;
>         }
>
>         wp = &wps->wp[offset / bs->bl.zone_size];
>         if (BDRV_ZT_IS_CONV(*wp)) {
>             /* Conventional zones do not have a write pointer */
>             goto unlock;
>         }
>
>         /* Return the written position for zone append */
>         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;
>         }
>
> unlock:
>         qemu_co_mutex_unlock(&wps->colock);
>     }
> #endif
>
> And making this entire block a helper function (e.g. advance_zone_wp()) would
> further clean the code. But that should be done in another patch. Care to send one ?

Sure. If replacing the current code block by saying advance_zone_wp(),
I guess this patch won't be necessary. So I will send another patch
(advance_zone_wp()...) after testing.

Sam