Re: [PATCH 2/2] hw/scsi/scsi-disk: Avoid buffer overrun parsing 'loadparam'

[Paolo Bonzini]

On 11/20/24 09:53, Philippe Mathieu-Daudé wrote:
> @@ -112,7 +113,7 @@ struct SCSIDiskState {
>       char *vendor;
>       char *product;
>       char *device_id;
> -    char *loadparm;     /* only for s390x */
> +    char loadparm[LOADPARM_LEN]; /* only for s390x */

You would need a +1 here because of

static char *scsi_property_get_loadparm(Object *obj, Error **errp)
{
    return g_strdup(SCSI_DISK_BASE(obj)->loadparm);
}

expecting NUL-termination as well.

> -    lp_str = g_malloc0(strlen(value));

I have sent a pull request that simply adds the +1 here, also because...

> -    if (!qdev_prop_sanitize_s390x_loadparm(lp_str, value, errp)) {
> -        g_free(lp_str);
> -        return;
> -    }
> -    SCSI_DISK_BASE(obj)->loadparm = lp_str;
> +    qdev_prop_sanitize_s390x_loadparm(SCSI_DISK_BASE(obj)->loadparm, value, 
> errp);

... this would overwrite SCSI_DISK_BASE(obj)->loadparm in case of error. 
 Note how the code is setting loadparm only after a successful 
qdev_prop_sanitize_s390x_loadparm.  That's not a problem in practice, 
because failing to set a property is usually fatal, but not good style 
either.

Thanks,

Paolo