Re: [PATCH v3 3/4] migration/file: fix type mismatch and NULL deref in multifd_file_recv_data

[Fabiano Rosas <farosas@suse.de>]

Junjie Cao <junjie.cao@intel.com> writes:

> multifd_file_recv_data() stores the return value of qio_channel_pread()
> (ssize_t) in a size_t variable.  On I/O error the -1 return value wraps
> to SIZE_MAX, producing a nonsensical read size in the error message.
>
> More critically, a short read (0 <= ret < data->size) is possible when
> the migration file is truncated.  In that case qio_channel_pread()
> returns a non-negative value without setting *errp.  The function then
> calls error_prepend(errp, ...) which dereferences *errp -- a NULL
> pointer -- crashing QEMU.
>
> Fix both issues by switching to qio_channel_pread_all() introduced in
> a previous patch, which retries on short reads and treats end-of-file
> as an error, so the caller no longer needs to check the byte count
> manually.  Add ERRP_GUARD() so that error_prepend() works correctly
> even when errp is &error_fatal or NULL.
>
> Fixes: a49d15a38d3d ("migration/multifd: Support incoming mapped-ram stream format")
> Suggested-by: Peter Xu <peterx@redhat.com>
> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
> Signed-off-by: Junjie Cao <junjie.cao@intel.com>
> ---
>  migration/file.c | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/migration/file.c b/migration/file.c
> index 5618aced49..0853188601 100644
> --- a/migration/file.c
> +++ b/migration/file.c
> @@ -254,15 +254,16 @@ int file_write_ramblock_iov(QIOChannel *ioc, const struct iovec *iov,
>  
>  int multifd_file_recv_data(MultiFDRecvParams *p, Error **errp)
>  {
> +    ERRP_GUARD();
>      MultiFDRecvData *data = p->data;
> -    size_t ret;
> +    int ret;
>  
> -    ret = qio_channel_pread(p->c, (char *) data->opaque,
> -                            data->size, data->file_offset, errp);
> -    if (ret != data->size) {
> +    ret = qio_channel_pread_all(p->c, (char *) data->opaque,
> +                                data->size, data->file_offset, errp);
> +    if (ret != 0) {
>          error_prepend(errp,
> -                      "multifd recv (%u): read 0x%zx, expected 0x%zx",
> -                      p->id, ret, data->size);
> +                      "multifd recv (%u): ",
> +                      p->id);
>          return -1;
>      }

Reviewed-by: Fabiano Rosas <farosas@suse.de>