| Message ID | 20241120085300.49866-3-philmd@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | hw/scsi/scsi-disk: Avoid buffer overrun parsing loadparam (CID 1565746) | expand |
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
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: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.
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.
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)
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(-)