Coverity reported a 1 byte overrun in scsi_property_set_loadparm
(CID 15657462). Since loadparam[] length is known, simply directly
allocate it in the device state.

Fixes: 429442e52d ("hw: Add 'loadparm' property to scsi disk devices")
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/scsi/scsi-disk.c | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index 96a09fe170..f6d6b7c1ea 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -38,6 +38,7 @@
 #include "hw/block/block.h"
 #include "hw/qdev-properties.h"
 #include "hw/qdev-properties-system.h"
+#include "hw/s390x/ipl/qipl.h"
 #include "sysemu/dma.h"
 #include "sysemu/sysemu.h"
 #include "qemu/cutils.h"
@@ -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 */
     bool tray_open;
     bool tray_locked;
     /*
@@ -3145,19 +3146,12 @@ static char *scsi_property_get_loadparm(Object *obj, Error **errp)
 static void scsi_property_set_loadparm(Object *obj, const char *value,
                                        Error **errp)
 {
-    char *lp_str;
-
     if (object_property_get_int(obj, "bootindex", NULL) < 0) {
         error_setg(errp, "'loadparm' is only valid for boot devices");
         return;
     }
 
-    lp_str = g_malloc0(strlen(value));
-    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);
 }
 
 static void scsi_property_add_specifics(DeviceClass *dc)
-- 
2.45.2

Am 20.11.2024 um 09:53 hat Philippe Mathieu-Daudé geschrieben:
> Coverity reported a 1 byte overrun in scsi_property_set_loadparm
> (CID 15657462). Since loadparam[] length is known, simply directly
> allocate it in the device state.
> 
> Fixes: 429442e52d ("hw: Add 'loadparm' property to scsi disk devices")
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Paolo already sent a pull request for a different fix (just allocating
one byte more). I think that's the better approach because other users
might expect the string to actually be null terminated.

Such as scsi_property_get_loadparm(), which you forgot to update:

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

Kevin

On 20/11/24 10:20, Kevin Wolf wrote:
> Am 20.11.2024 um 09:53 hat Philippe Mathieu-Daudé geschrieben:
>> Coverity reported a 1 byte overrun in scsi_property_set_loadparm
>> (CID 15657462). Since loadparam[] length is known, simply directly
>> allocate it in the device state.
>>
>> Fixes: 429442e52d ("hw: Add 'loadparm' property to scsi disk devices")
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> 
> Paolo already sent a pull request for a different fix (just allocating
> one byte more). I think that's the better approach because other users
> might expect the string to actually be null terminated.
> 
> Such as scsi_property_get_loadparm(), which you forgot to update:
> 
>      static char *scsi_property_get_loadparm(Object *obj, Error **errp)
>      {
>          return g_strdup(SCSI_DISK_BASE(obj)->loadparm);
>      }

Yeah I missed that.

Maybe consider the first patch as cleanup for 10.0? I can repost later.

Regards,

Phil.

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

On 20/11/24 10:10, Paolo Bonzini wrote:
> 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.

I guess I was not well awake :) Please disregard this patch.