| Message ID | 20241120085300.49866-2-philmd@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | hw/scsi/scsi-disk: Avoid buffer overrun parsing loadparam (CID 1565746) | expand |
ping for this single cleanup patch? On 20/11/24 09:52, Philippe Mathieu-Daudé wrote: > "hw/s390x/ipl/qipl.h" defines loadparm[] length as LOADPARM_LEN, > use that instead of the magic '8' value. Use a char type for > char buffer. > > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> > --- > hw/s390x/ccw-device.h | 5 +++-- > hw/s390x/ipl.h | 2 +- > include/hw/qdev-properties-system.h | 2 +- > include/hw/s390x/s390-virtio-ccw.h | 3 ++- > hw/core/qdev-properties-system.c | 8 +++++--- > hw/s390x/ipl.c | 6 +++--- > hw/scsi/scsi-disk.c | 2 +- > 7 files changed, 16 insertions(+), 12 deletions(-) > > diff --git a/hw/s390x/ccw-device.h b/hw/s390x/ccw-device.h > index 4439feb140..94a9b35714 100644 > --- a/hw/s390x/ccw-device.h > +++ b/hw/s390x/ccw-device.h > @@ -15,6 +15,7 @@ > #include "hw/qdev-core.h" > #include "hw/s390x/css.h" > #include "hw/s390x/css-bridge.h" > +#include "hw/s390x/ipl/qipl.h" > > struct CcwDevice { > DeviceState parent_obj; > @@ -27,7 +28,7 @@ struct CcwDevice { > /* The actual busid of the virtual subchannel. */ > CssDevId subch_id; > /* If set, use this loadparm value when device is boot target */ > - uint8_t loadparm[8]; > + char loadparm[LOADPARM_LEN]; > }; > typedef struct CcwDevice CcwDevice; > > @@ -54,6 +55,6 @@ OBJECT_DECLARE_TYPE(CcwDevice, CCWDeviceClass, CCW_DEVICE) > extern const PropertyInfo ccw_loadparm; > > #define DEFINE_PROP_CCW_LOADPARM(_n, _s, _f) \ > - DEFINE_PROP(_n, _s, _f, ccw_loadparm, typeof(uint8_t[8])) > + DEFINE_PROP(_n, _s, _f, ccw_loadparm, typeof(char[LOADPARM_LEN])) > > #endif > diff --git a/hw/s390x/ipl.h b/hw/s390x/ipl.h > index d7d0b7bfd2..014b530ad2 100644 > --- a/hw/s390x/ipl.h > +++ b/hw/s390x/ipl.h > @@ -23,7 +23,7 @@ > #define MAX_BOOT_DEVS 8 /* Max number of devices that may have a bootindex */ > > void s390_ipl_convert_loadparm(char *ascii_lp, uint8_t *ebcdic_lp); > -void s390_ipl_fmt_loadparm(uint8_t *loadparm, char *str, Error **errp); > +void s390_ipl_fmt_loadparm(char *loadparm, char *str, Error **errp); > void s390_rebuild_iplb(uint16_t index, IplParameterBlock *iplb); > void s390_ipl_update_diag308(IplParameterBlock *iplb); > int s390_ipl_prepare_pv_header(Error **errp); > diff --git a/include/hw/qdev-properties-system.h b/include/hw/qdev-properties-system.h > index 7ec37f6316..844af7a200 100644 > --- a/include/hw/qdev-properties-system.h > +++ b/include/hw/qdev-properties-system.h > @@ -3,7 +3,7 @@ > > #include "hw/qdev-properties.h" > > -bool qdev_prop_sanitize_s390x_loadparm(uint8_t *loadparm, const char *str, > +bool qdev_prop_sanitize_s390x_loadparm(char *loadparm, const char *str, > Error **errp); > > extern const PropertyInfo qdev_prop_chr; > diff --git a/include/hw/s390x/s390-virtio-ccw.h b/include/hw/s390x/s390-virtio-ccw.h > index 996864a34e..9d4e00b0c7 100644 > --- a/include/hw/s390x/s390-virtio-ccw.h > +++ b/include/hw/s390x/s390-virtio-ccw.h > @@ -14,6 +14,7 @@ > #include "hw/boards.h" > #include "qom/object.h" > #include "hw/s390x/sclp.h" > +#include "hw/s390x/ipl/qipl.h" > > #define TYPE_S390_CCW_MACHINE "s390-ccw-machine" > > @@ -28,7 +29,7 @@ struct S390CcwMachineState { > bool aes_key_wrap; > bool dea_key_wrap; > bool pv; > - uint8_t loadparm[8]; > + char loadparm[LOADPARM_LEN]; > > SCLPDevice *sclp; > }; > diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c > index a61c5ee6dd..e8e9cd8e04 100644 > --- a/hw/core/qdev-properties-system.c > +++ b/hw/core/qdev-properties-system.c > @@ -35,6 +35,7 @@ > #include "hw/pci/pci.h" > #include "hw/pci/pcie.h" > #include "hw/i386/x86.h" > +#include "hw/s390x/ipl/qipl.h" > #include "util/block-helpers.h" > > static bool check_prop_still_unset(Object *obj, const char *name, > @@ -58,14 +59,15 @@ static bool check_prop_still_unset(Object *obj, const char *name, > return false; > } > > -bool qdev_prop_sanitize_s390x_loadparm(uint8_t *loadparm, const char *str, > +bool qdev_prop_sanitize_s390x_loadparm(char *loadparm, const char *str, > Error **errp) > { > int i, len; > > len = strlen(str); > - if (len > 8) { > - error_setg(errp, "'loadparm' can only contain up to 8 characters"); > + if (len > LOADPARM_LEN) { > + error_setg(errp, "'loadparm' can only contain up to %u characters", > + LOADPARM_LEN); > return false; > } > > diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c > index 30734661ad..6fd3774c7d 100644 > --- a/hw/s390x/ipl.c > +++ b/hw/s390x/ipl.c > @@ -416,7 +416,7 @@ static uint64_t s390_ipl_map_iplb_chain(IplParameterBlock *iplb_chain) > return chain_addr; > } > > -void s390_ipl_fmt_loadparm(uint8_t *loadparm, char *str, Error **errp) > +void s390_ipl_fmt_loadparm(char *loadparm, char *str, Error **errp) > { > /* Initialize the loadparm with spaces */ > memset(loadparm, ' ', LOADPARM_LEN); > @@ -439,8 +439,8 @@ static bool s390_build_iplb(DeviceState *dev_st, IplParameterBlock *iplb) > CcwDevice *ccw_dev = NULL; > SCSIDevice *sd; > int devtype; > - uint8_t *lp; > - g_autofree void *scsi_lp = NULL; > + char *lp; > + g_autofree char *scsi_lp = NULL; > > /* > * Currently allow IPL only from CCW devices. > diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c > index 8e553487d5..96a09fe170 100644 > --- a/hw/scsi/scsi-disk.c > +++ b/hw/scsi/scsi-disk.c > @@ -3145,7 +3145,7 @@ static char *scsi_property_get_loadparm(Object *obj, Error **errp) > static void scsi_property_set_loadparm(Object *obj, const char *value, > Error **errp) > { > - void *lp_str; > + char *lp_str; > > if (object_property_get_int(obj, "bootindex", NULL) < 0) { > error_setg(errp, "'loadparm' is only valid for boot devices");
On 30/12/2024 18.02, Philippe Mathieu-Daudé wrote: > ping for this single cleanup patch? > > On 20/11/24 09:52, Philippe Mathieu-Daudé wrote: >> "hw/s390x/ipl/qipl.h" defines loadparm[] length as LOADPARM_LEN, >> use that instead of the magic '8' value. Use a char type for >> char buffer. >> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> >> --- >> hw/s390x/ccw-device.h | 5 +++-- >> hw/s390x/ipl.h | 2 +- >> include/hw/qdev-properties-system.h | 2 +- >> include/hw/s390x/s390-virtio-ccw.h | 3 ++- >> hw/core/qdev-properties-system.c | 8 +++++--- >> hw/s390x/ipl.c | 6 +++--- >> hw/scsi/scsi-disk.c | 2 +- >> 7 files changed, 16 insertions(+), 12 deletions(-) >> >> diff --git a/hw/s390x/ccw-device.h b/hw/s390x/ccw-device.h >> index 4439feb140..94a9b35714 100644 >> --- a/hw/s390x/ccw-device.h >> +++ b/hw/s390x/ccw-device.h >> @@ -15,6 +15,7 @@ >> #include "hw/qdev-core.h" >> #include "hw/s390x/css.h" >> #include "hw/s390x/css-bridge.h" >> +#include "hw/s390x/ipl/qipl.h" >> struct CcwDevice { >> DeviceState parent_obj; >> @@ -27,7 +28,7 @@ struct CcwDevice { >> /* The actual busid of the virtual subchannel. */ >> CssDevId subch_id; >> /* If set, use this loadparm value when device is boot target */ >> - uint8_t loadparm[8]; >> + char loadparm[LOADPARM_LEN]; I'm not sure whether changing the type is such a good idea ... some spots that handle loadparm include EBCDIC encoding, and the unsigned type might have been used here for a reason? I'd better keep uint8_t unless you've double-checked all combinations... Thomas
On 3/1/25 18:17, Thomas Huth wrote: > On 30/12/2024 18.02, Philippe Mathieu-Daudé wrote: >> ping for this single cleanup patch? >> >> On 20/11/24 09:52, Philippe Mathieu-Daudé wrote: >>> "hw/s390x/ipl/qipl.h" defines loadparm[] length as LOADPARM_LEN, >>> use that instead of the magic '8' value. Use a char type for >>> char buffer. >>> >>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> >>> --- >>> hw/s390x/ccw-device.h | 5 +++-- >>> hw/s390x/ipl.h | 2 +- >>> include/hw/qdev-properties-system.h | 2 +- >>> include/hw/s390x/s390-virtio-ccw.h | 3 ++- >>> hw/core/qdev-properties-system.c | 8 +++++--- >>> hw/s390x/ipl.c | 6 +++--- >>> hw/scsi/scsi-disk.c | 2 +- >>> 7 files changed, 16 insertions(+), 12 deletions(-) >>> >>> diff --git a/hw/s390x/ccw-device.h b/hw/s390x/ccw-device.h >>> index 4439feb140..94a9b35714 100644 >>> --- a/hw/s390x/ccw-device.h >>> +++ b/hw/s390x/ccw-device.h >>> @@ -15,6 +15,7 @@ >>> #include "hw/qdev-core.h" >>> #include "hw/s390x/css.h" >>> #include "hw/s390x/css-bridge.h" >>> +#include "hw/s390x/ipl/qipl.h" >>> struct CcwDevice { >>> DeviceState parent_obj; >>> @@ -27,7 +28,7 @@ struct CcwDevice { >>> /* The actual busid of the virtual subchannel. */ >>> CssDevId subch_id; >>> /* If set, use this loadparm value when device is boot target */ >>> - uint8_t loadparm[8]; >>> + char loadparm[LOADPARM_LEN]; > > I'm not sure whether changing the type is such a good idea ... some > spots that handle loadparm include EBCDIC encoding, and the unsigned > type might have been used here for a reason? I'd better keep uint8_t > unless you've double-checked all combinations... OK, no problem, thanks for checking! Regards, Phil.
diff --git a/hw/s390x/ccw-device.h b/hw/s390x/ccw-device.h index 4439feb140..94a9b35714 100644 --- a/hw/s390x/ccw-device.h +++ b/hw/s390x/ccw-device.h @@ -15,6 +15,7 @@ #include "hw/qdev-core.h" #include "hw/s390x/css.h" #include "hw/s390x/css-bridge.h" +#include "hw/s390x/ipl/qipl.h" struct CcwDevice { DeviceState parent_obj; @@ -27,7 +28,7 @@ struct CcwDevice { /* The actual busid of the virtual subchannel. */ CssDevId subch_id; /* If set, use this loadparm value when device is boot target */ - uint8_t loadparm[8]; + char loadparm[LOADPARM_LEN]; }; typedef struct CcwDevice CcwDevice; @@ -54,6 +55,6 @@ OBJECT_DECLARE_TYPE(CcwDevice, CCWDeviceClass, CCW_DEVICE) extern const PropertyInfo ccw_loadparm; #define DEFINE_PROP_CCW_LOADPARM(_n, _s, _f) \ - DEFINE_PROP(_n, _s, _f, ccw_loadparm, typeof(uint8_t[8])) + DEFINE_PROP(_n, _s, _f, ccw_loadparm, typeof(char[LOADPARM_LEN])) #endif diff --git a/hw/s390x/ipl.h b/hw/s390x/ipl.h index d7d0b7bfd2..014b530ad2 100644 --- a/hw/s390x/ipl.h +++ b/hw/s390x/ipl.h @@ -23,7 +23,7 @@ #define MAX_BOOT_DEVS 8 /* Max number of devices that may have a bootindex */ void s390_ipl_convert_loadparm(char *ascii_lp, uint8_t *ebcdic_lp); -void s390_ipl_fmt_loadparm(uint8_t *loadparm, char *str, Error **errp); +void s390_ipl_fmt_loadparm(char *loadparm, char *str, Error **errp); void s390_rebuild_iplb(uint16_t index, IplParameterBlock *iplb); void s390_ipl_update_diag308(IplParameterBlock *iplb); int s390_ipl_prepare_pv_header(Error **errp); diff --git a/include/hw/qdev-properties-system.h b/include/hw/qdev-properties-system.h index 7ec37f6316..844af7a200 100644 --- a/include/hw/qdev-properties-system.h +++ b/include/hw/qdev-properties-system.h @@ -3,7 +3,7 @@ #include "hw/qdev-properties.h" -bool qdev_prop_sanitize_s390x_loadparm(uint8_t *loadparm, const char *str, +bool qdev_prop_sanitize_s390x_loadparm(char *loadparm, const char *str, Error **errp); extern const PropertyInfo qdev_prop_chr; diff --git a/include/hw/s390x/s390-virtio-ccw.h b/include/hw/s390x/s390-virtio-ccw.h index 996864a34e..9d4e00b0c7 100644 --- a/include/hw/s390x/s390-virtio-ccw.h +++ b/include/hw/s390x/s390-virtio-ccw.h @@ -14,6 +14,7 @@ #include "hw/boards.h" #include "qom/object.h" #include "hw/s390x/sclp.h" +#include "hw/s390x/ipl/qipl.h" #define TYPE_S390_CCW_MACHINE "s390-ccw-machine" @@ -28,7 +29,7 @@ struct S390CcwMachineState { bool aes_key_wrap; bool dea_key_wrap; bool pv; - uint8_t loadparm[8]; + char loadparm[LOADPARM_LEN]; SCLPDevice *sclp; }; diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c index a61c5ee6dd..e8e9cd8e04 100644 --- a/hw/core/qdev-properties-system.c +++ b/hw/core/qdev-properties-system.c @@ -35,6 +35,7 @@ #include "hw/pci/pci.h" #include "hw/pci/pcie.h" #include "hw/i386/x86.h" +#include "hw/s390x/ipl/qipl.h" #include "util/block-helpers.h" static bool check_prop_still_unset(Object *obj, const char *name, @@ -58,14 +59,15 @@ static bool check_prop_still_unset(Object *obj, const char *name, return false; } -bool qdev_prop_sanitize_s390x_loadparm(uint8_t *loadparm, const char *str, +bool qdev_prop_sanitize_s390x_loadparm(char *loadparm, const char *str, Error **errp) { int i, len; len = strlen(str); - if (len > 8) { - error_setg(errp, "'loadparm' can only contain up to 8 characters"); + if (len > LOADPARM_LEN) { + error_setg(errp, "'loadparm' can only contain up to %u characters", + LOADPARM_LEN); return false; } diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c index 30734661ad..6fd3774c7d 100644 --- a/hw/s390x/ipl.c +++ b/hw/s390x/ipl.c @@ -416,7 +416,7 @@ static uint64_t s390_ipl_map_iplb_chain(IplParameterBlock *iplb_chain) return chain_addr; } -void s390_ipl_fmt_loadparm(uint8_t *loadparm, char *str, Error **errp) +void s390_ipl_fmt_loadparm(char *loadparm, char *str, Error **errp) { /* Initialize the loadparm with spaces */ memset(loadparm, ' ', LOADPARM_LEN); @@ -439,8 +439,8 @@ static bool s390_build_iplb(DeviceState *dev_st, IplParameterBlock *iplb) CcwDevice *ccw_dev = NULL; SCSIDevice *sd; int devtype; - uint8_t *lp; - g_autofree void *scsi_lp = NULL; + char *lp; + g_autofree char *scsi_lp = NULL; /* * Currently allow IPL only from CCW devices. diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c index 8e553487d5..96a09fe170 100644 --- a/hw/scsi/scsi-disk.c +++ b/hw/scsi/scsi-disk.c @@ -3145,7 +3145,7 @@ static char *scsi_property_get_loadparm(Object *obj, Error **errp) static void scsi_property_set_loadparm(Object *obj, const char *value, Error **errp) { - void *lp_str; + char *lp_str; if (object_property_get_int(obj, "bootindex", NULL) < 0) { error_setg(errp, "'loadparm' is only valid for boot devices");
"hw/s390x/ipl/qipl.h" defines loadparm[] length as LOADPARM_LEN, use that instead of the magic '8' value. Use a char type for char buffer. Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> --- hw/s390x/ccw-device.h | 5 +++-- hw/s390x/ipl.h | 2 +- include/hw/qdev-properties-system.h | 2 +- include/hw/s390x/s390-virtio-ccw.h | 3 ++- hw/core/qdev-properties-system.c | 8 +++++--- hw/s390x/ipl.c | 6 +++--- hw/scsi/scsi-disk.c | 2 +- 7 files changed, 16 insertions(+), 12 deletions(-)