| Message ID | 1337721011-20842-1-git-send-email-vianac@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
On 22 May 2012 22:10, Crístian Viana <vianac@linux.vnet.ibm.com> wrote: > Based on the following conversation: > > http://mid.gmane.org/4F69F05B.5010500@codemonkey.ws > >> Which reminds me - qemu sticks the release version in >> guest visible places like CPU version. >> This is wrong and causes windows guests to print messages >> about driver updates when you switch. >> We should find all these places and stop doing this. > > There is a new field on the struct QEmuMachine, hw_version, which may > contain the version that the specific machine should report. If that > field is set, then that machine will report that version to the virtual > machine. OK, this has been bugging me for the last three versions, and since I'm complaining about other things anyway: can you reword this commit message, please, so that it is a standalone paragraph explaining (a) what the commit does and (b) why it is doing it, rather than being a combination of an unattributed quote from Anthony and some new text from you. Commit messages aren't emails. Explanatory remarks like the URL to the previous discussion can go below the '---' line where they won't appear in the formal git commit message. > Signed-off-by: Crístian Viana <vianac@linux.vnet.ibm.com> > --- When you're posting a new version of a patch please include a summary of changes since the previous version below the '---' line. > diff --git a/hw/nseries.c b/hw/nseries.c > index a5cfa8c..9d07cb8 100644 > --- a/hw/nseries.c > +++ b/hw/nseries.c > @@ -1247,7 +1247,7 @@ static int n8x0_atag_setup(void *p, int model) > stw_raw(w ++, 24); /* u16 len */ > strcpy((void *) w, "hw-build"); /* char component[12] */ > w += 6; > - strcpy((void *) w, "QEMU " QEMU_VERSION); /* char version[12] */ > + sprintf((void *) w, "QEMU %s", qemu_get_version()); /* char version[12] */ So when you posted the previous version of your patch it was pointed out that this is a buffer overflow: http://lists.gnu.org/archive/html/qemu-devel/2012-04/msg01657.html You need to fix this. > diff --git a/hw/usb/redirect.c b/hw/usb/redirect.c > index 51c27b4..28c2b05 100644 > --- a/hw/usb/redirect.c > +++ b/hw/usb/redirect.c > @@ -143,8 +143,6 @@ static void usbredir_interrupt_packet(void *priv, uint32_t id, > static int usbredir_handle_status(USBRedirDevice *dev, > int status, int actual_len); > > -#define VERSION "qemu usb-redir guest " QEMU_VERSION > - > /* > * Logging stuff > */ > @@ -794,6 +792,9 @@ static void usbredir_open_close_bh(void *opaque) > { > USBRedirDevice *dev = opaque; > uint32_t caps[USB_REDIR_CAPS_SIZE] = { 0, }; > + char version[32]; > + > + snprintf(version, sizeof(version), "qemu usb-redir guest %s", qemu_get_version()); The questions about the usb-redir version still apply too: http://lists.gnu.org/archive/html/qemu-devel/2012-04/msg01700.html Don't just leave things unchanged from previous versions that were objected to without explicitly mentioning them in the below-the-'---' commentary (ie explaining why you decided not to change them), please. Otherwise reviewers have to go through the whole thing with a fine tooth comb rechecking whether you've actually fixed anything. thanks -- PMM
Hi Peter, Thanks for all your tips! > OK, this has been bugging me for the last three versions, and > since I'm complaining about other things anyway: can you reword > this commit message, please, so that it is a standalone paragraph > explaining (a) what the commit does and (b) why it is doing it, rather > than being a combination of an unattributed quote from Anthony and some > new text from you. Commit messages aren't emails. > Explanatory remarks like the URL to the previous discussion can go > below the '---' line where they won't appear in the formal git commit > message. Ok, I rewrote the commit message. > When you're posting a new version of a patch please include a > summary of changes since the previous version below the '---' > line. Ok. > So when you posted the previous version of your patch it was pointed > out that this is a buffer overflow: > http://lists.gnu.org/archive/html/qemu-devel/2012-04/msg01657.html > > You need to fix this. I have sent a reply to that thread explaining that the user actually doesn't have control of that string, that is only used internally in the code (just like the QEMU_VERSION macro). I fixed the code now with snprintf copying at most 12 chars to the string (the array size). I can't think of why pstrcat would be better in this case, as suggested by Erik. > The questions about the usb-redir version still apply too: > http://lists.gnu.org/archive/html/qemu-devel/2012-04/msg01700.html I have also sent a reply to that thread asking if the usb-redir version appears to the guest but I haven't got no answer. > Don't just leave things unchanged from previous versions that were objected > to without explicitly mentioning them in the below-the-'---' commentary (ie > explaining why you decided not to change them), please. Otherwise reviewers > have to go through the whole thing with a fine tooth comb rechecking whether > you've actually fixed anything. I didn't know there was "patch changelog protocol" here, I'll do it from now on. Best regards, Crístian.
On 05/23/2012 10:08 AM, Crístian Viana wrote: >> So when you posted the previous version of your patch it was pointed >> out that this is a buffer overflow: >> http://lists.gnu.org/archive/html/qemu-devel/2012-04/msg01657.html >> >> You need to fix this. > > I have sent a reply to that thread explaining that the user actually > doesn't have control of that string, that is only used internally in the > code (just like the QEMU_VERSION macro). > I fixed the code now with snprintf copying at most 12 chars to the > string (the array size). I can't think of why pstrcat would be better in > this case, as suggested by Erik. s/Erik/Eric/, but you're not the first to make that typo. pstrcat is more efficient than snprintf() - the former is dedicated to a single task, while the latter has to parse a format string and decode that it is doing a single %s expansion. In other words, just because *printf can do string concatenation doesn't make it the best tool for the job.
On 23-05-2012 13:11, Eric Blake wrote: > pstrcat is more efficient than snprintf() - the former is dedicated to a > single task, while the latter has to parse a format string and decode > that it is doing a single %s expansion. In other words, just because > *printf can do string concatenation doesn't make it the best tool for > the job. This would be the new code: snprintf((void *) w, 12, "QEMU %s", qemu_get_version()); /* char version[12] */ I'm not sure of what value the pointer contains at that moment, concatenating doesn't seem safe to me. What if w already contains a string? The result won't be the same. I don't understand the Nokia code, so I prefer to leave it as it was before (with snprintf). Best regards, Crístian.
On 23 May 2012 21:06, Crístian Viana <vianac@linux.vnet.ibm.com> wrote: > This would be the new code: > > snprintf((void *) w, 12, "QEMU %s", qemu_get_version()); /* char version[12] */ > > I'm not sure of what value the pointer contains at that moment, > concatenating doesn't seem safe to me. What if w already contains a string? > The result won't be the same. The point is that your snprintf is not actually using the full power of a format string parser, it's just concatenating two strings ("QEMU " and the version). The simple way to put two strings into a buffer one after the other is to copy string A and then concatenate string B on the end. > I don't understand the Nokia code, so I prefer > to leave it as it was before (with snprintf). The Nokia code as it was before doesn't use sprintf or snprintf. We're just filling in a buffer in memory, and we have a char[12] array, as the comment says. (NB that w is an int16_t*, which is why w+=6 moves us over the array.) What you want is something like: strcpy((void*)w, "QEMU "); pstrcat((void*)w, 12, qemu_get_version()); instead of the current single strcpy(). > I don't understand the Nokia code, so I prefer > to leave it as it was before (with snprintf). The Nokia code as it was before doesn't use sprintf or snprintf. -- PMM
On 23-05-2012 17:54, Peter Maydell wrote: > The point is that your snprintf is not actually using the full power of > a format string parser, it's just concatenating two strings ("QEMU " > and the version). The simple way to put two strings into a buffer > one after the other is to copy string A and then concatenate string > B on the end. I got it. That has been fixed in three snippets of my patch, thanks! Best regards, Crístian.
diff --git a/hw/boards.h b/hw/boards.h index 667177d..59c01d0 100644 --- a/hw/boards.h +++ b/hw/boards.h @@ -29,6 +29,7 @@ typedef struct QEMUMachine { const char *default_machine_opts; GlobalProperty *compat_props; struct QEMUMachine *next; + const char *hw_version; } QEMUMachine; int qemu_register_machine(QEMUMachine *m); diff --git a/hw/bt-sdp.c b/hw/bt-sdp.c index 3e390ab..c0431d1 100644 --- a/hw/bt-sdp.c +++ b/hw/bt-sdp.c @@ -834,7 +834,7 @@ SERVICE(hid, ATTRIBUTE(DOC_URL, URL("http://bellard.org/qemu/user-doc.html")) ATTRIBUTE(SVCNAME_PRIMARY, STRING("QEMU Bluetooth HID")) ATTRIBUTE(SVCDESC_PRIMARY, STRING("QEMU Keyboard/Mouse")) - ATTRIBUTE(SVCPROV_PRIMARY, STRING("QEMU " QEMU_VERSION)) + ATTRIBUTE(SVCPROV_PRIMARY, STRING("QEMU")) /* Profile specific */ ATTRIBUTE(DEVICE_RELEASE_NUMBER, UINT16(0x0091)) /* Deprecated, remove */ @@ -908,7 +908,7 @@ SERVICE(sdp, LIST(UUID128(SDP_SERVER_PROFILE_ID) UINT16(0x0100)) )) ATTRIBUTE(DOC_URL, URL("http://bellard.org/qemu/user-doc.html")) - ATTRIBUTE(SVCPROV_PRIMARY, STRING("QEMU " QEMU_VERSION)) + ATTRIBUTE(SVCPROV_PRIMARY, STRING("QEMU")) /* Profile specific */ ATTRIBUTE(VERSION_NUM_LIST, LIST(UINT16(0x0100))) @@ -931,7 +931,7 @@ SERVICE(pnp, LIST(UUID128(PNP_INFO_PROFILE_ID) UINT16(0x0100)) )) ATTRIBUTE(DOC_URL, URL("http://bellard.org/qemu/user-doc.html")) - ATTRIBUTE(SVCPROV_PRIMARY, STRING("QEMU " QEMU_VERSION)) + ATTRIBUTE(SVCPROV_PRIMARY, STRING("QEMU")) /* Profile specific */ ATTRIBUTE(SPECIFICATION_ID, UINT16(0x0100)) diff --git a/hw/ide/core.c b/hw/ide/core.c index 9785d5f..c6de8a2 100644 --- a/hw/ide/core.c +++ b/hw/ide/core.c @@ -1983,7 +1983,7 @@ int ide_init_drive(IDEState *s, BlockDriverState *bs, IDEDriveKind kind, if (version) { pstrcpy(s->version, sizeof(s->version), version); } else { - pstrcpy(s->version, sizeof(s->version), QEMU_VERSION); + pstrcpy(s->version, sizeof(s->version), qemu_get_version()); } ide_reset(s); diff --git a/hw/nseries.c b/hw/nseries.c index a5cfa8c..9d07cb8 100644 --- a/hw/nseries.c +++ b/hw/nseries.c @@ -1247,7 +1247,7 @@ static int n8x0_atag_setup(void *p, int model) stw_raw(w ++, 24); /* u16 len */ strcpy((void *) w, "hw-build"); /* char component[12] */ w += 6; - strcpy((void *) w, "QEMU " QEMU_VERSION); /* char version[12] */ + sprintf((void *) w, "QEMU %s", qemu_get_version()); /* char version[12] */ w += 6; tag = (model == 810) ? "1.1.10-qemu" : "1.1.6-qemu"; diff --git a/hw/pc_piix.c b/hw/pc_piix.c index a7aad4b..67aa10d 100644 --- a/hw/pc_piix.c +++ b/hw/pc_piix.c @@ -392,6 +392,7 @@ static QEMUMachine pc_machine_v1_0 = { PC_COMPAT_1_0, { /* end of list */ } }, + .hw_version = "1.0", }; #define PC_COMPAT_0_15 \ @@ -406,6 +407,7 @@ static QEMUMachine pc_machine_v0_15 = { PC_COMPAT_0_15, { /* end of list */ } }, + .hw_version = "0.15", }; #define PC_COMPAT_0_14 \ @@ -446,6 +448,7 @@ static QEMUMachine pc_machine_v0_14 = { }, { /* end of list */ } }, + .hw_version = "0.14", }; #define PC_COMPAT_0_13 \ @@ -482,6 +485,7 @@ static QEMUMachine pc_machine_v0_13 = { }, { /* end of list */ } }, + .hw_version = "0.13", }; #define PC_COMPAT_0_12 \ @@ -513,7 +517,8 @@ static QEMUMachine pc_machine_v0_12 = { .value = stringify(0), }, { /* end of list */ } - } + }, + .hw_version = "0.12", }; #define PC_COMPAT_0_11 \ @@ -545,7 +550,8 @@ static QEMUMachine pc_machine_v0_11 = { .value = "0.11", }, { /* end of list */ } - } + }, + .hw_version = "0.11", }; static QEMUMachine pc_machine_v0_10 = { @@ -578,6 +584,7 @@ static QEMUMachine pc_machine_v0_10 = { }, { /* end of list */ } }, + .hw_version = "0.10", }; static QEMUMachine isapc_machine = { diff --git a/hw/scsi-bus.c b/hw/scsi-bus.c index 8ab9bcd..316cd2c 100644 --- a/hw/scsi-bus.c +++ b/hw/scsi-bus.c @@ -406,7 +406,7 @@ static bool scsi_target_emulate_inquiry(SCSITargetReq *r) r->buf[7] = 0x10 | (r->req.bus->info->tcq ? 0x02 : 0); /* Sync, TCQ. */ memcpy(&r->buf[8], "QEMU ", 8); memcpy(&r->buf[16], "QEMU TARGET ", 16); - strncpy((char *) &r->buf[32], QEMU_VERSION, 4); + strncpy((char *) &r->buf[32], qemu_get_version(), 4); } return true; } diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c index 045c764..19bc4ac 100644 --- a/hw/scsi-disk.c +++ b/hw/scsi-disk.c @@ -1704,7 +1704,7 @@ static int scsi_initfn(SCSIDevice *dev) } if (!s->version) { - s->version = g_strdup(QEMU_VERSION); + s->version = g_strdup(qemu_get_version()); } if (bdrv_is_sg(s->qdev.conf.bs)) { diff --git a/hw/usb/dev-bluetooth.c b/hw/usb/dev-bluetooth.c index 6b74eff..55bc191 100644 --- a/hw/usb/dev-bluetooth.c +++ b/hw/usb/dev-bluetooth.c @@ -57,7 +57,7 @@ enum { }; static const USBDescStrings desc_strings = { - [STR_MANUFACTURER] = "QEMU " QEMU_VERSION, + [STR_MANUFACTURER] = "QEMU", [STR_SERIALNUMBER] = "1", }; diff --git a/hw/usb/dev-hid.c b/hw/usb/dev-hid.c index f29544d..b3dcd23 100644 --- a/hw/usb/dev-hid.c +++ b/hw/usb/dev-hid.c @@ -60,7 +60,7 @@ enum { }; static const USBDescStrings desc_strings = { - [STR_MANUFACTURER] = "QEMU " QEMU_VERSION, + [STR_MANUFACTURER] = "QEMU", [STR_PRODUCT_MOUSE] = "QEMU USB Mouse", [STR_PRODUCT_TABLET] = "QEMU USB Tablet", [STR_PRODUCT_KEYBOARD] = "QEMU USB Keyboard", diff --git a/hw/usb/dev-hub.c b/hw/usb/dev-hub.c index b5962da..8fd30df 100644 --- a/hw/usb/dev-hub.c +++ b/hw/usb/dev-hub.c @@ -90,7 +90,7 @@ enum { }; static const USBDescStrings desc_strings = { - [STR_MANUFACTURER] = "QEMU " QEMU_VERSION, + [STR_MANUFACTURER] = "QEMU", [STR_PRODUCT] = "QEMU USB Hub", [STR_SERIALNUMBER] = "314159", }; diff --git a/hw/usb/dev-serial.c b/hw/usb/dev-serial.c index 56743ee..8aa6552 100644 --- a/hw/usb/dev-serial.c +++ b/hw/usb/dev-serial.c @@ -111,7 +111,7 @@ enum { }; static const USBDescStrings desc_strings = { - [STR_MANUFACTURER] = "QEMU " QEMU_VERSION, + [STR_MANUFACTURER] = "QEMU", [STR_PRODUCT_SERIAL] = "QEMU USB SERIAL", [STR_PRODUCT_BRAILLE] = "QEMU USB BRAILLE", [STR_SERIALNUMBER] = "1", diff --git a/hw/usb/dev-smartcard-reader.c b/hw/usb/dev-smartcard-reader.c index 3b7604e..b0e70b0 100644 --- a/hw/usb/dev-smartcard-reader.c +++ b/hw/usb/dev-smartcard-reader.c @@ -81,7 +81,7 @@ do { \ #define CCID_CONTROL_GET_DATA_RATES 0x3 #define CCID_PRODUCT_DESCRIPTION "QEMU USB CCID" -#define CCID_VENDOR_DESCRIPTION "QEMU " QEMU_VERSION +#define CCID_VENDOR_DESCRIPTION "QEMU" #define CCID_INTERFACE_NAME "CCID Interface" #define CCID_SERIAL_NUMBER_STRING "1" /* @@ -401,7 +401,7 @@ enum { }; static const USBDescStrings desc_strings = { - [STR_MANUFACTURER] = "QEMU " QEMU_VERSION, + [STR_MANUFACTURER] = "QEMU", [STR_PRODUCT] = "QEMU USB CCID", [STR_SERIALNUMBER] = "1", [STR_INTERFACE] = "CCID Interface", diff --git a/hw/usb/dev-storage.c b/hw/usb/dev-storage.c index ae22fb1..f863dfc 100644 --- a/hw/usb/dev-storage.c +++ b/hw/usb/dev-storage.c @@ -82,7 +82,7 @@ enum { }; static const USBDescStrings desc_strings = { - [STR_MANUFACTURER] = "QEMU " QEMU_VERSION, + [STR_MANUFACTURER] = "QEMU", [STR_PRODUCT] = "QEMU USB HARDDRIVE", [STR_SERIALNUMBER] = "1", [STR_CONFIG_FULL] = "Full speed config (usb 1.1)", diff --git a/hw/usb/dev-wacom.c b/hw/usb/dev-wacom.c index 3b51d45..ed9a5ee 100644 --- a/hw/usb/dev-wacom.c +++ b/hw/usb/dev-wacom.c @@ -62,7 +62,7 @@ enum { }; static const USBDescStrings desc_strings = { - [STR_MANUFACTURER] = "QEMU " QEMU_VERSION, + [STR_MANUFACTURER] = "QEMU", [STR_PRODUCT] = "Wacom PenPartner", [STR_SERIALNUMBER] = "1", }; diff --git a/hw/usb/redirect.c b/hw/usb/redirect.c index 51c27b4..28c2b05 100644 --- a/hw/usb/redirect.c +++ b/hw/usb/redirect.c @@ -143,8 +143,6 @@ static void usbredir_interrupt_packet(void *priv, uint32_t id, static int usbredir_handle_status(USBRedirDevice *dev, int status, int actual_len); -#define VERSION "qemu usb-redir guest " QEMU_VERSION - /* * Logging stuff */ @@ -794,6 +792,9 @@ static void usbredir_open_close_bh(void *opaque) { USBRedirDevice *dev = opaque; uint32_t caps[USB_REDIR_CAPS_SIZE] = { 0, }; + char version[32]; + + snprintf(version, sizeof(version), "qemu usb-redir guest %s", qemu_get_version()); usbredir_device_disconnect(dev); @@ -828,7 +829,7 @@ static void usbredir_open_close_bh(void *opaque) usbredirparser_caps_set_cap(caps, usb_redir_cap_connect_device_version); usbredirparser_caps_set_cap(caps, usb_redir_cap_filter); - usbredirparser_init(dev->parser, VERSION, caps, USB_REDIR_CAPS_SIZE, 0); + usbredirparser_init(dev->parser, version, caps, USB_REDIR_CAPS_SIZE, 0); usbredirparser_do_write(dev->parser); } } diff --git a/osdep.c b/osdep.c index 3e6bada..03817f0 100644 --- a/osdep.c +++ b/osdep.c @@ -48,6 +48,8 @@ extern int madvise(caddr_t, size_t, int); #include "trace.h" #include "qemu_socket.h" +static const char *qemu_version = QEMU_VERSION; + int socket_set_cork(int fd, int v) { #if defined(SOL_TCP) && defined(TCP_CORK) @@ -242,3 +244,12 @@ ssize_t qemu_recv_full(int fd, void *buf, size_t count, int flags) return total; } +void qemu_set_version(const char *version) +{ + qemu_version = version; +} + +const char *qemu_get_version(void) +{ + return qemu_version; +} diff --git a/osdep.h b/osdep.h index 9db8766..3ea4af0 100644 --- a/osdep.h +++ b/osdep.h @@ -149,4 +149,7 @@ static inline void qemu_timersub(const struct timeval *val1, void qemu_set_cloexec(int fd); +void qemu_set_version(const char *); +const char *qemu_get_version(void); + #endif diff --git a/target-i386/cpu.c b/target-i386/cpu.c index 89b4ac7..3d03bfe 100644 --- a/target-i386/cpu.c +++ b/target-i386/cpu.c @@ -303,7 +303,6 @@ static x86_def_t builtin_x86_defs[] = { .ext3_features = CPUID_EXT3_LAHF_LM | CPUID_EXT3_SVM | CPUID_EXT3_ABM | CPUID_EXT3_SSE4A, .xlevel = 0x8000000A, - .model_id = "QEMU Virtual CPU version " QEMU_VERSION, }, { .name = "phenom", @@ -386,7 +385,6 @@ static x86_def_t builtin_x86_defs[] = { .features = PPRO_FEATURES, .ext_features = CPUID_EXT_SSE3 | CPUID_EXT_POPCNT, .xlevel = 0x80000004, - .model_id = "QEMU Virtual CPU version " QEMU_VERSION, }, { .name = "kvm32", @@ -465,8 +463,6 @@ static x86_def_t builtin_x86_defs[] = { .features = PPRO_FEATURES | CPUID_PSE36 | CPUID_VME | CPUID_MTRR | CPUID_MCA, .ext2_features = (PPRO_FEATURES & EXT2_FEATURE_MASK) | CPUID_EXT2_MMXEXT | CPUID_EXT2_3DNOW | CPUID_EXT2_3DNOWEXT, .xlevel = 0x80000008, - /* XXX: put another string ? */ - .model_id = "QEMU Virtual CPU version " QEMU_VERSION, }, { .name = "n270", @@ -1324,11 +1320,24 @@ void cpu_clear_apic_feature(CPUX86State *env) */ void x86_cpudef_setup(void) { - int i; + int i, j; + static const char *model_with_versions[] = { "qemu32", "qemu64", "athlon" }; for (i = 0; i < ARRAY_SIZE(builtin_x86_defs); ++i) { builtin_x86_defs[i].next = x86_defs; builtin_x86_defs[i].flags = 1; + + /* Look for specific "cpudef" models that */ + /* have the QEmu version in .model_id */ + for (j = 0; j < ARRAY_SIZE(model_with_versions); j++) { + if (strcmp(model_with_versions[j], builtin_x86_defs[i].name) == 0) { + snprintf(builtin_x86_defs[i].model_id, + sizeof(builtin_x86_defs[i].model_id), + "QEMU Virtual CPU version %s", qemu_get_version()); + break; + } + } + x86_defs = &builtin_x86_defs[i]; } #if !defined(CONFIG_USER_ONLY) diff --git a/vl.c b/vl.c index 23ab3a3..668917c 100644 --- a/vl.c +++ b/vl.c @@ -3208,6 +3208,10 @@ int main(int argc, char **argv, char **envp) } loc_set_none(); + if (machine->hw_version) { + qemu_set_version(machine->hw_version); + } + /* Init CPU def lists, based on config * - Must be called after all the qemu_read_config_file() calls * - Must be called before list_cpus()