| Message ID | 1308562518-21322-1-git-send-email-armbru@redhat.com |
|---|---|
| State | New |
| Headers | show |
Markus Armbruster <armbru@redhat.com> writes: > It needs to be a qdev property, because it belongs to the drive's > guest part. Precedence: commit a0fef654 and 6ced55a5. > > Bonus: info qtree now shows the serial number. Ping?
Am 20.06.2011 11:35, schrieb Markus Armbruster: > It needs to be a qdev property, because it belongs to the drive's > guest part. Precedence: commit a0fef654 and 6ced55a5. > > Bonus: info qtree now shows the serial number. > > Signed-off-by: Markus Armbruster <armbru@redhat.com> > --- > hw/s390-virtio-bus.c | 4 +++- > hw/s390-virtio-bus.h | 1 + > hw/virtio-blk.c | 29 +++++++++++++++++++---------- > hw/virtio-blk.h | 2 ++ > hw/virtio-pci.c | 4 +++- > hw/virtio-pci.h | 1 + > hw/virtio.h | 3 ++- > 7 files changed, 31 insertions(+), 13 deletions(-) > > diff --git a/hw/s390-virtio-bus.c b/hw/s390-virtio-bus.c > index d4a12f7..2bf4821 100644 > --- a/hw/s390-virtio-bus.c > +++ b/hw/s390-virtio-bus.c > @@ -128,7 +128,8 @@ static int s390_virtio_blk_init(VirtIOS390Device *dev) > { > VirtIODevice *vdev; > > - vdev = virtio_blk_init((DeviceState *)dev, &dev->block); > + vdev = virtio_blk_init((DeviceState *)dev, &dev->block, > + &dev->block_serial); > if (!vdev) { > return -1; > } > @@ -355,6 +356,7 @@ static VirtIOS390DeviceInfo s390_virtio_blk = { > .qdev.size = sizeof(VirtIOS390Device), > .qdev.props = (Property[]) { > DEFINE_BLOCK_PROPERTIES(VirtIOS390Device, block), > + DEFINE_PROP_STRING("serial", VirtIOS390Device, block_serial), > DEFINE_PROP_END_OF_LIST(), > }, > }; > diff --git a/hw/s390-virtio-bus.h b/hw/s390-virtio-bus.h > index 0c412d0..f1bece7 100644 > --- a/hw/s390-virtio-bus.h > +++ b/hw/s390-virtio-bus.h > @@ -42,6 +42,7 @@ typedef struct VirtIOS390Device { > uint8_t feat_len; > VirtIODevice *vdev; > BlockConf block; > + char *block_serial; > NICConf nic; > uint32_t host_features; > virtio_serial_conf serial; > diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c > index 91e0394..6471ac8 100644 > --- a/hw/virtio-blk.c > +++ b/hw/virtio-blk.c > @@ -28,8 +28,8 @@ typedef struct VirtIOBlock > void *rq; > QEMUBH *bh; > BlockConf *conf; > + char *serial; > unsigned short sector_mask; > - char sn[BLOCK_SERIAL_STRLEN]; > DeviceState *qdev; > } VirtIOBlock; > > @@ -362,8 +362,13 @@ static void virtio_blk_handle_request(VirtIOBlockReq *req, > } else if (type & VIRTIO_BLK_T_GET_ID) { > VirtIOBlock *s = req->dev; > > - memcpy(req->elem.in_sg[0].iov_base, s->sn, > - MIN(req->elem.in_sg[0].iov_len, sizeof(s->sn))); > + /* > + * NB: per existing s/n string convention the string is > + * terminated by '\0' only when shorter than buffer. > + */ > + strncpy(req->elem.in_sg[0].iov_base, > + s->serial ? s->serial : "", > + MIN(req->elem.in_sg[0].iov_len, VIRTIO_BLK_ID_BYTES)); Not sure what you're trying to do with VIRTIO_BLK_ID_BYTES here. s->string either is dinfo->serial, in which case it happens to be the same as BLOCK_SERIAL_STRLEN and as such makes some sense. Or it may be a qdev property, in which case the string just has the length it has. Or it's the empty string. So I think in two of three cases you're potentially reading beyond the end of the buffer. Kevin
Kevin Wolf <kwolf@redhat.com> writes: > Am 20.06.2011 11:35, schrieb Markus Armbruster: >> It needs to be a qdev property, because it belongs to the drive's >> guest part. Precedence: commit a0fef654 and 6ced55a5. >> >> Bonus: info qtree now shows the serial number. >> >> Signed-off-by: Markus Armbruster <armbru@redhat.com> >> --- >> hw/s390-virtio-bus.c | 4 +++- >> hw/s390-virtio-bus.h | 1 + >> hw/virtio-blk.c | 29 +++++++++++++++++++---------- >> hw/virtio-blk.h | 2 ++ >> hw/virtio-pci.c | 4 +++- >> hw/virtio-pci.h | 1 + >> hw/virtio.h | 3 ++- >> 7 files changed, 31 insertions(+), 13 deletions(-) >> >> diff --git a/hw/s390-virtio-bus.c b/hw/s390-virtio-bus.c >> index d4a12f7..2bf4821 100644 >> --- a/hw/s390-virtio-bus.c >> +++ b/hw/s390-virtio-bus.c >> @@ -128,7 +128,8 @@ static int s390_virtio_blk_init(VirtIOS390Device *dev) >> { >> VirtIODevice *vdev; >> >> - vdev = virtio_blk_init((DeviceState *)dev, &dev->block); >> + vdev = virtio_blk_init((DeviceState *)dev, &dev->block, >> + &dev->block_serial); >> if (!vdev) { >> return -1; >> } >> @@ -355,6 +356,7 @@ static VirtIOS390DeviceInfo s390_virtio_blk = { >> .qdev.size = sizeof(VirtIOS390Device), >> .qdev.props = (Property[]) { >> DEFINE_BLOCK_PROPERTIES(VirtIOS390Device, block), >> + DEFINE_PROP_STRING("serial", VirtIOS390Device, block_serial), >> DEFINE_PROP_END_OF_LIST(), >> }, >> }; >> diff --git a/hw/s390-virtio-bus.h b/hw/s390-virtio-bus.h >> index 0c412d0..f1bece7 100644 >> --- a/hw/s390-virtio-bus.h >> +++ b/hw/s390-virtio-bus.h >> @@ -42,6 +42,7 @@ typedef struct VirtIOS390Device { >> uint8_t feat_len; >> VirtIODevice *vdev; >> BlockConf block; >> + char *block_serial; >> NICConf nic; >> uint32_t host_features; >> virtio_serial_conf serial; >> diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c >> index 91e0394..6471ac8 100644 >> --- a/hw/virtio-blk.c >> +++ b/hw/virtio-blk.c >> @@ -28,8 +28,8 @@ typedef struct VirtIOBlock >> void *rq; >> QEMUBH *bh; >> BlockConf *conf; >> + char *serial; >> unsigned short sector_mask; >> - char sn[BLOCK_SERIAL_STRLEN]; >> DeviceState *qdev; >> } VirtIOBlock; >> >> @@ -362,8 +362,13 @@ static void virtio_blk_handle_request(VirtIOBlockReq *req, >> } else if (type & VIRTIO_BLK_T_GET_ID) { >> VirtIOBlock *s = req->dev; >> >> - memcpy(req->elem.in_sg[0].iov_base, s->sn, >> - MIN(req->elem.in_sg[0].iov_len, sizeof(s->sn))); >> + /* >> + * NB: per existing s/n string convention the string is >> + * terminated by '\0' only when shorter than buffer. >> + */ >> + strncpy(req->elem.in_sg[0].iov_base, >> + s->serial ? s->serial : "", >> + MIN(req->elem.in_sg[0].iov_len, VIRTIO_BLK_ID_BYTES)); > > Not sure what you're trying to do with VIRTIO_BLK_ID_BYTES here. > > s->string either is dinfo->serial, in which case it happens to be the You mean s->serial, don't you? > same as BLOCK_SERIAL_STRLEN and as such makes some sense. Or it may be a > qdev property, in which case the string just has the length it has. Or > it's the empty string. So I think in two of three cases you're > potentially reading beyond the end of the buffer. I can't see that. strncpy(dst, src, n) reads up to n characters or the terminating zero, whatever comes first. strncpy()'s second argument is zero-terminated. "" trivially is. s->serial is a malloc'ed, zero-terminated string. It's set either by qdev_prop_string's parse_string(), or two patch hunks down (some snipped text restored for your convenience). In both cases, the value is freshly strdup'ed. Therefore, strncpy() won't read beyond the buffer. strncpy(dst, src, n) writes exactly n characters. Its third argument is the minimum of the buffer size and VIRTIO_BLK_ID_BYTES. The old code works the same, only it uses sizeof(s->sn) instead of VIRTIO_BLK_ID_BYTES, with sn defined as "char sn[BLOCK_SERIAL_STRLEN]". VIRTIO_BLK_ID_BYTES is really part of the virtio protocol. Its value indeed happens to be the same as BLOCK_SERIAL_STRLEN, but I chose not to rely on that. Matches IDE and SCSI, only they use literals instead of defines. The kernel has it in include/linux/virtio_blk.h. Our partial copy of that header is hw/virtio-blk.h. I added VIRTIO_BLK_ID_BYTES to that copy (four patch hunks down). >> @@ -531,7 +536,8 @@ static void virtio_blk_change_cb(void *opaque, int reason) >> } >> } >> >> -VirtIODevice *virtio_blk_init(DeviceState *dev, BlockConf *conf) >> +VirtIODevice *virtio_blk_init(DeviceState *dev, BlockConf *conf, >> + char **serial) >> { >> VirtIOBlock *s; >> int cylinders, heads, secs; >> @@ -547,6 +553,14 @@ VirtIODevice *virtio_blk_init(DeviceState *dev, BlockConf *conf) >> return NULL; >> } >> >> + if (!*serial) { >> + /* try to fall back to value set with legacy -drive serial=... */ >> + dinfo = drive_get_by_blockdev(conf->bs); >> + if (*dinfo->serial) { >> + *serial = strdup(dinfo->serial); >> + } >> + } >> + >> s = (VirtIOBlock *)virtio_common_init("virtio-blk", VIRTIO_ID_BLOCK, >> sizeof(struct virtio_blk_config), >> sizeof(VirtIOBlock)); >> >> @@ -556,16 +570,11 @@ VirtIODevice *virtio_blk_init(DeviceState *dev, BlockConf *conf) >> s->vdev.reset = virtio_blk_reset; >> s->bs = conf->bs; >> s->conf = conf; >> + s->serial = *serial; >> s->rq = NULL; >> s->sector_mask = (s->conf->logical_block_size / BDRV_SECTOR_SIZE) - 1; >> bdrv_guess_geometry(s->bs, &cylinders, &heads, &secs); >> >> - /* NB: per existing s/n string convention the string is terminated >> - * by '\0' only when less than sizeof (s->sn) >> - */ >> - dinfo = drive_get_by_blockdev(s->bs); >> - strncpy(s->sn, dinfo->serial, sizeof (s->sn)); >> - >> s->vq = virtio_add_queue(&s->vdev, 128, virtio_blk_handle_output); >> >> qemu_add_vm_change_state_handler(virtio_blk_dma_restart_cb, s); >> diff --git a/hw/virtio-blk.h b/hw/virtio-blk.h >> index fff46da..5645d2b 100644 >> --- a/hw/virtio-blk.h >> +++ b/hw/virtio-blk.h >> @@ -34,6 +34,8 @@ >> #define VIRTIO_BLK_F_WCACHE 9 /* write cache enabled */ >> #define VIRTIO_BLK_F_TOPOLOGY 10 /* Topology information is available */ >> >> +#define VIRTIO_BLK_ID_BYTES 20 /* ID string length */ >> + >> struct virtio_blk_config >> { >> uint64_t capacity;
Am 04.07.2011 13:29, schrieb Markus Armbruster: > Kevin Wolf <kwolf@redhat.com> writes: > >> Am 20.06.2011 11:35, schrieb Markus Armbruster: >>> It needs to be a qdev property, because it belongs to the drive's >>> guest part. Precedence: commit a0fef654 and 6ced55a5. >>> >>> Bonus: info qtree now shows the serial number. >>> >>> Signed-off-by: Markus Armbruster <armbru@redhat.com> >>> --- >>> hw/s390-virtio-bus.c | 4 +++- >>> hw/s390-virtio-bus.h | 1 + >>> hw/virtio-blk.c | 29 +++++++++++++++++++---------- >>> hw/virtio-blk.h | 2 ++ >>> hw/virtio-pci.c | 4 +++- >>> hw/virtio-pci.h | 1 + >>> hw/virtio.h | 3 ++- >>> 7 files changed, 31 insertions(+), 13 deletions(-) >>> >>> diff --git a/hw/s390-virtio-bus.c b/hw/s390-virtio-bus.c >>> index d4a12f7..2bf4821 100644 >>> --- a/hw/s390-virtio-bus.c >>> +++ b/hw/s390-virtio-bus.c >>> @@ -128,7 +128,8 @@ static int s390_virtio_blk_init(VirtIOS390Device *dev) >>> { >>> VirtIODevice *vdev; >>> >>> - vdev = virtio_blk_init((DeviceState *)dev, &dev->block); >>> + vdev = virtio_blk_init((DeviceState *)dev, &dev->block, >>> + &dev->block_serial); >>> if (!vdev) { >>> return -1; >>> } >>> @@ -355,6 +356,7 @@ static VirtIOS390DeviceInfo s390_virtio_blk = { >>> .qdev.size = sizeof(VirtIOS390Device), >>> .qdev.props = (Property[]) { >>> DEFINE_BLOCK_PROPERTIES(VirtIOS390Device, block), >>> + DEFINE_PROP_STRING("serial", VirtIOS390Device, block_serial), >>> DEFINE_PROP_END_OF_LIST(), >>> }, >>> }; >>> diff --git a/hw/s390-virtio-bus.h b/hw/s390-virtio-bus.h >>> index 0c412d0..f1bece7 100644 >>> --- a/hw/s390-virtio-bus.h >>> +++ b/hw/s390-virtio-bus.h >>> @@ -42,6 +42,7 @@ typedef struct VirtIOS390Device { >>> uint8_t feat_len; >>> VirtIODevice *vdev; >>> BlockConf block; >>> + char *block_serial; >>> NICConf nic; >>> uint32_t host_features; >>> virtio_serial_conf serial; >>> diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c >>> index 91e0394..6471ac8 100644 >>> --- a/hw/virtio-blk.c >>> +++ b/hw/virtio-blk.c >>> @@ -28,8 +28,8 @@ typedef struct VirtIOBlock >>> void *rq; >>> QEMUBH *bh; >>> BlockConf *conf; >>> + char *serial; >>> unsigned short sector_mask; >>> - char sn[BLOCK_SERIAL_STRLEN]; >>> DeviceState *qdev; >>> } VirtIOBlock; >>> >>> @@ -362,8 +362,13 @@ static void virtio_blk_handle_request(VirtIOBlockReq *req, >>> } else if (type & VIRTIO_BLK_T_GET_ID) { >>> VirtIOBlock *s = req->dev; >>> >>> - memcpy(req->elem.in_sg[0].iov_base, s->sn, >>> - MIN(req->elem.in_sg[0].iov_len, sizeof(s->sn))); >>> + /* >>> + * NB: per existing s/n string convention the string is >>> + * terminated by '\0' only when shorter than buffer. >>> + */ >>> + strncpy(req->elem.in_sg[0].iov_base, >>> + s->serial ? s->serial : "", >>> + MIN(req->elem.in_sg[0].iov_len, VIRTIO_BLK_ID_BYTES)); >> >> Not sure what you're trying to do with VIRTIO_BLK_ID_BYTES here. >> >> s->string either is dinfo->serial, in which case it happens to be the > > You mean s->serial, don't you? > >> same as BLOCK_SERIAL_STRLEN and as such makes some sense. Or it may be a >> qdev property, in which case the string just has the length it has. Or >> it's the empty string. So I think in two of three cases you're >> potentially reading beyond the end of the buffer. > > I can't see that. You're right, sorry for the noise. What confused me is that I didn't expect some limit in the protocol (and if any, then certainly not 20), so I started making wild guesses what this might be used for, and reading strncpy as memcpy because it made more sense with the guesses... I should just stop reviewing patches early in the morning. :-) Applied the patch to the block branch. Kevin
diff --git a/hw/s390-virtio-bus.c b/hw/s390-virtio-bus.c index d4a12f7..2bf4821 100644 --- a/hw/s390-virtio-bus.c +++ b/hw/s390-virtio-bus.c @@ -128,7 +128,8 @@ static int s390_virtio_blk_init(VirtIOS390Device *dev) { VirtIODevice *vdev; - vdev = virtio_blk_init((DeviceState *)dev, &dev->block); + vdev = virtio_blk_init((DeviceState *)dev, &dev->block, + &dev->block_serial); if (!vdev) { return -1; } @@ -355,6 +356,7 @@ static VirtIOS390DeviceInfo s390_virtio_blk = { .qdev.size = sizeof(VirtIOS390Device), .qdev.props = (Property[]) { DEFINE_BLOCK_PROPERTIES(VirtIOS390Device, block), + DEFINE_PROP_STRING("serial", VirtIOS390Device, block_serial), DEFINE_PROP_END_OF_LIST(), }, }; diff --git a/hw/s390-virtio-bus.h b/hw/s390-virtio-bus.h index 0c412d0..f1bece7 100644 --- a/hw/s390-virtio-bus.h +++ b/hw/s390-virtio-bus.h @@ -42,6 +42,7 @@ typedef struct VirtIOS390Device { uint8_t feat_len; VirtIODevice *vdev; BlockConf block; + char *block_serial; NICConf nic; uint32_t host_features; virtio_serial_conf serial; diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c index 91e0394..6471ac8 100644 --- a/hw/virtio-blk.c +++ b/hw/virtio-blk.c @@ -28,8 +28,8 @@ typedef struct VirtIOBlock void *rq; QEMUBH *bh; BlockConf *conf; + char *serial; unsigned short sector_mask; - char sn[BLOCK_SERIAL_STRLEN]; DeviceState *qdev; } VirtIOBlock; @@ -362,8 +362,13 @@ static void virtio_blk_handle_request(VirtIOBlockReq *req, } else if (type & VIRTIO_BLK_T_GET_ID) { VirtIOBlock *s = req->dev; - memcpy(req->elem.in_sg[0].iov_base, s->sn, - MIN(req->elem.in_sg[0].iov_len, sizeof(s->sn))); + /* + * NB: per existing s/n string convention the string is + * terminated by '\0' only when shorter than buffer. + */ + strncpy(req->elem.in_sg[0].iov_base, + s->serial ? s->serial : "", + MIN(req->elem.in_sg[0].iov_len, VIRTIO_BLK_ID_BYTES)); virtio_blk_req_complete(req, VIRTIO_BLK_S_OK); } else if (type & VIRTIO_BLK_T_OUT) { qemu_iovec_init_external(&req->qiov, &req->elem.out_sg[1], @@ -531,7 +536,8 @@ static void virtio_blk_change_cb(void *opaque, int reason) } } -VirtIODevice *virtio_blk_init(DeviceState *dev, BlockConf *conf) +VirtIODevice *virtio_blk_init(DeviceState *dev, BlockConf *conf, + char **serial) { VirtIOBlock *s; int cylinders, heads, secs; @@ -547,6 +553,14 @@ VirtIODevice *virtio_blk_init(DeviceState *dev, BlockConf *conf) return NULL; } + if (!*serial) { + /* try to fall back to value set with legacy -drive serial=... */ + dinfo = drive_get_by_blockdev(conf->bs); + if (*dinfo->serial) { + *serial = strdup(dinfo->serial); + } + } + s = (VirtIOBlock *)virtio_common_init("virtio-blk", VIRTIO_ID_BLOCK, sizeof(struct virtio_blk_config), sizeof(VirtIOBlock)); @@ -556,16 +570,11 @@ VirtIODevice *virtio_blk_init(DeviceState *dev, BlockConf *conf) s->vdev.reset = virtio_blk_reset; s->bs = conf->bs; s->conf = conf; + s->serial = *serial; s->rq = NULL; s->sector_mask = (s->conf->logical_block_size / BDRV_SECTOR_SIZE) - 1; bdrv_guess_geometry(s->bs, &cylinders, &heads, &secs); - /* NB: per existing s/n string convention the string is terminated - * by '\0' only when less than sizeof (s->sn) - */ - dinfo = drive_get_by_blockdev(s->bs); - strncpy(s->sn, dinfo->serial, sizeof (s->sn)); - s->vq = virtio_add_queue(&s->vdev, 128, virtio_blk_handle_output); qemu_add_vm_change_state_handler(virtio_blk_dma_restart_cb, s); diff --git a/hw/virtio-blk.h b/hw/virtio-blk.h index fff46da..5645d2b 100644 --- a/hw/virtio-blk.h +++ b/hw/virtio-blk.h @@ -34,6 +34,8 @@ #define VIRTIO_BLK_F_WCACHE 9 /* write cache enabled */ #define VIRTIO_BLK_F_TOPOLOGY 10 /* Topology information is available */ +#define VIRTIO_BLK_ID_BYTES 20 /* ID string length */ + struct virtio_blk_config { uint64_t capacity; diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c index c018351..a8c236e 100644 --- a/hw/virtio-pci.c +++ b/hw/virtio-pci.c @@ -710,7 +710,8 @@ static int virtio_blk_init_pci(PCIDevice *pci_dev) proxy->class_code != PCI_CLASS_STORAGE_OTHER) proxy->class_code = PCI_CLASS_STORAGE_SCSI; - vdev = virtio_blk_init(&pci_dev->qdev, &proxy->block); + vdev = virtio_blk_init(&pci_dev->qdev, &proxy->block, + &proxy->block_serial); if (!vdev) { return -1; } @@ -825,6 +826,7 @@ static PCIDeviceInfo virtio_info[] = { .qdev.props = (Property[]) { DEFINE_PROP_HEX32("class", VirtIOPCIProxy, class_code, 0), DEFINE_BLOCK_PROPERTIES(VirtIOPCIProxy, block), + DEFINE_PROP_STRING("serial", VirtIOPCIProxy, block_serial), DEFINE_PROP_BIT("ioeventfd", VirtIOPCIProxy, flags, VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT, true), DEFINE_PROP_UINT32("vectors", VirtIOPCIProxy, nvectors, 2), diff --git a/hw/virtio-pci.h b/hw/virtio-pci.h index a4b5fd3..bc29864 100644 --- a/hw/virtio-pci.h +++ b/hw/virtio-pci.h @@ -26,6 +26,7 @@ typedef struct { uint32_t class_code; uint32_t nvectors; BlockConf block; + char *block_serial; NICConf nic; uint32_t host_features; #ifdef CONFIG_LINUX diff --git a/hw/virtio.h b/hw/virtio.h index bc72289..583600d 100644 --- a/hw/virtio.h +++ b/hw/virtio.h @@ -192,7 +192,8 @@ void virtio_bind_device(VirtIODevice *vdev, const VirtIOBindings *binding, void *opaque); /* Base devices. */ -VirtIODevice *virtio_blk_init(DeviceState *dev, BlockConf *conf); +VirtIODevice *virtio_blk_init(DeviceState *dev, BlockConf *conf, + char **serial); struct virtio_net_conf; VirtIODevice *virtio_net_init(DeviceState *dev, NICConf *conf, struct virtio_net_conf *net);
It needs to be a qdev property, because it belongs to the drive's guest part. Precedence: commit a0fef654 and 6ced55a5. Bonus: info qtree now shows the serial number. Signed-off-by: Markus Armbruster <armbru@redhat.com> --- hw/s390-virtio-bus.c | 4 +++- hw/s390-virtio-bus.h | 1 + hw/virtio-blk.c | 29 +++++++++++++++++++---------- hw/virtio-blk.h | 2 ++ hw/virtio-pci.c | 4 +++- hw/virtio-pci.h | 1 + hw/virtio.h | 3 ++- 7 files changed, 31 insertions(+), 13 deletions(-)