Coverity reports a possible buffer overrun due to a non-NUL-terminated
string in scsi_property_set_loadparm().  While things are not so easy,
because qdev_prop_sanitize_s390x_loadparm is designed to operate on a
buffer that is not NUL-terminated, in this case the string *does* have
to be NUL-terminated because it is read by scsi_property_get_loadparm
and s390_build_iplb.

Cc: jrossi@linux.ibm.com
Cc: thuth@redhat.com
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/scsi/scsi-disk.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index 8e553487d50..7f13b0588f2 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -3152,7 +3152,7 @@ static void scsi_property_set_loadparm(Object *obj, const char *value,
         return;
     }
 
-    lp_str = g_malloc0(strlen(value));
+    lp_str = g_malloc0(strlen(value) + 1);
     if (!qdev_prop_sanitize_s390x_loadparm(lp_str, value, errp)) {
         g_free(lp_str);
         return;
-- 
2.47.0

On 19/11/2024 22.31, Paolo Bonzini wrote:
> Coverity reports a possible buffer overrun due to a non-NUL-terminated
> string in scsi_property_set_loadparm().  While things are not so easy,
> because qdev_prop_sanitize_s390x_loadparm is designed to operate on a
> buffer that is not NUL-terminated, in this case the string *does* have
> to be NUL-terminated because it is read by scsi_property_get_loadparm
> and s390_build_iplb.
> 
> Cc: jrossi@linux.ibm.com
> Cc: thuth@redhat.com
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Fixes: 429442e52d ("hw: Add "loadparm" property to scsi disk devices for 
booting on s390x")

> diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
> index 8e553487d50..7f13b0588f2 100644
> --- a/hw/scsi/scsi-disk.c
> +++ b/hw/scsi/scsi-disk.c
> @@ -3152,7 +3152,7 @@ static void scsi_property_set_loadparm(Object *obj, const char *value,
>           return;
>       }
>   
> -    lp_str = g_malloc0(strlen(value));
> +    lp_str = g_malloc0(strlen(value) + 1);

D'oh, me looks for a brown paperbag for hiding... sorry for that one.

Reviewed-by: Thomas Huth <thuth@redhat.com>

On 11/19/24 4:31 PM, Paolo Bonzini wrote:
> Coverity reports a possible buffer overrun due to a non-NUL-terminated
> string in scsi_property_set_loadparm().  While things are not so easy,
> because qdev_prop_sanitize_s390x_loadparm is designed to operate on a
> buffer that is not NUL-terminated, in this case the string *does* have
> to be NUL-terminated because it is read by scsi_property_get_loadparm
> and s390_build_iplb.
>
> Cc: jrossi@linux.ibm.com
> Cc: thuth@redhat.com
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   hw/scsi/scsi-disk.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
> index 8e553487d50..7f13b0588f2 100644
> --- a/hw/scsi/scsi-disk.c
> +++ b/hw/scsi/scsi-disk.c
> @@ -3152,7 +3152,7 @@ static void scsi_property_set_loadparm(Object *obj, const char *value,
>           return;
>       }
>   
> -    lp_str = g_malloc0(strlen(value));
> +    lp_str = g_malloc0(strlen(value) + 1);
>       if (!qdev_prop_sanitize_s390x_loadparm(lp_str, value, errp)) {
>           g_free(lp_str);
>           return;
Reviewed-by Jared Rossi <jrossi@linux.ibm.com>