Re: [PATCH] kvm tools:Fix memory leakage in open all disks

[Alexandru Elisei <alexandru.elisei@arm.com>]

Hi,

Adding the kvmtool maintainers (you can find them in the README file).

On Tue, Jun 18, 2024 at 03:52:47PM +0800, leixiang wrote:
> Fix memory leakage in disk/core disk_image__open_all when malloc disk failed,
> should free the disks that already malloced.
> 
> Signed-off-by: Lei Xiang <leixiang@kylinos.cn>
> Suggested-by: Xie Ming <xieming@kylinos.cn>
> ---
>  disk/core.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/disk/core.c b/disk/core.c
> index dd2f258..affeece 100644
> --- a/disk/core.c
> +++ b/disk/core.c
> @@ -195,8 +195,10 @@ static struct disk_image **disk_image__open_all(struct kvm *kvm)
>  
>  		if (wwpn) {
>  			disks[i] = malloc(sizeof(struct disk_image));
> -			if (!disks[i])
> -				return ERR_PTR(-ENOMEM);
> +			if (!disks[i]) {
> +				err = ERR_PTR(-ENOMEM);
> +				goto error;
> +			}
>  			disks[i]->wwpn = wwpn;
>  			disks[i]->tpgt = tpgt;

Currently, the latest patch on branch master is ca31abf5d9c3 ("arm64: Allow
the user to select the max SVE vector length"), and struct disk_image
doesn't have a tpgt field. Did you write this patch on a local branch?

>  			continue;

This is what the 'error' label does:

error:
        for (i = 0; i < count; i++)
                if (!IS_ERR_OR_NULL(disks[i]))
                        disk_image__close(disks[i]);

        free(disks);
        return err;

And disk_image__close() ends up poking all sort of fields from struct
disk_image, including dereferencing pointers embedded in the struct. If
WWPN is specified for a disk, struct disk_image is allocated using malloc
as above, the field wwwpn is set and the rest of the fields are left
uninitialized. Because of this, calling disk_image__close() on a struct
disk_image with wwpn can lead to all sorts of nasty things happening.

May I suggest allocating disks[i] using calloc in the wwpn case to fix
this? Ideally, you would have two patches:

1. A patch that changes the disk[i] allocation to calloc(), to prevent
disk_image__close() accessing unitialized fields when disk_image__open()
fails after initialized a WWPN disk.

2. This patch.

Thanks,
Alex

> -- 
> 2.34.1
> 
>