* [Qemu-devel] [PATCH] block/iscsi: avoid potential overflow of acb->task->cdb
@ 2016-05-24 8:59 Peter Lieven
2016-05-31 6:44 ` Fam Zheng
0 siblings, 1 reply; 6+ messages in thread
From: Peter Lieven @ 2016-05-24 8:59 UTC (permalink / raw)
To: qemu-block; +Cc: qemu-devel, pbonzini, famz, Peter Lieven, qemu-stable
at least in the path via virtio-blk the maximum size is not
restricted.
Cc: qemu-stable@nongnu.org
Signed-off-by: Peter Lieven <pl@kamp.de>
---
block/iscsi.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/block/iscsi.c b/block/iscsi.c
index 2ca8e72..e7d5f7b 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -833,6 +833,13 @@ static BlockAIOCB *iscsi_aio_ioctl(BlockDriverState *bs,
return &acb->common;
}
+ if (acb->ioh->cmd_len > SCSI_CDB_MAX_SIZE) {
+ error_report("iSCSI: ioctl error CDB exceeds max size (%d > %d)",
+ acb->ioh->cmd_len, SCSI_CDB_MAX_SIZE);
+ qemu_aio_unref(acb);
+ return NULL;
+ }
+
acb->task = malloc(sizeof(struct scsi_task));
if (acb->task == NULL) {
error_report("iSCSI: Failed to allocate task for scsi command. %s",
--
1.9.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [Qemu-devel] [PATCH] block/iscsi: avoid potential overflow of acb->task->cdb
2016-05-24 8:59 [Qemu-devel] [PATCH] block/iscsi: avoid potential overflow of acb->task->cdb Peter Lieven
@ 2016-05-31 6:44 ` Fam Zheng
2016-05-31 7:35 ` [Qemu-devel] [Qemu-block] " Kevin Wolf
2016-05-31 7:39 ` Kevin Wolf
0 siblings, 2 replies; 6+ messages in thread
From: Fam Zheng @ 2016-05-31 6:44 UTC (permalink / raw)
To: Peter Lieven; +Cc: qemu-block, pbonzini, qemu-devel, qemu-stable
On Tue, 05/24 10:59, Peter Lieven wrote:
> at least in the path via virtio-blk the maximum size is not
> restricted.
>
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Peter Lieven <pl@kamp.de>
> ---
> block/iscsi.c | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> diff --git a/block/iscsi.c b/block/iscsi.c
> index 2ca8e72..e7d5f7b 100644
> --- a/block/iscsi.c
> +++ b/block/iscsi.c
> @@ -833,6 +833,13 @@ static BlockAIOCB *iscsi_aio_ioctl(BlockDriverState *bs,
> return &acb->common;
> }
>
> + if (acb->ioh->cmd_len > SCSI_CDB_MAX_SIZE) {
> + error_report("iSCSI: ioctl error CDB exceeds max size (%d > %d)",
> + acb->ioh->cmd_len, SCSI_CDB_MAX_SIZE);
> + qemu_aio_unref(acb);
> + return NULL;
> + }
> +
> acb->task = malloc(sizeof(struct scsi_task));
> if (acb->task == NULL) {
> error_report("iSCSI: Failed to allocate task for scsi command. %s",
Is it better to invoke the cb and report -EINVAL to the caller?
Fam
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Qemu-devel] [Qemu-block] [PATCH] block/iscsi: avoid potential overflow of acb->task->cdb
2016-05-31 6:44 ` Fam Zheng
@ 2016-05-31 7:35 ` Kevin Wolf
2016-05-31 8:01 ` Fam Zheng
2016-05-31 7:39 ` Kevin Wolf
1 sibling, 1 reply; 6+ messages in thread
From: Kevin Wolf @ 2016-05-31 7:35 UTC (permalink / raw)
To: Fam Zheng; +Cc: Peter Lieven, pbonzini, qemu-devel, qemu-block, qemu-stable
Am 31.05.2016 um 08:44 hat Fam Zheng geschrieben:
> On Tue, 05/24 10:59, Peter Lieven wrote:
> > at least in the path via virtio-blk the maximum size is not
> > restricted.
> >
> > Cc: qemu-stable@nongnu.org
> > Signed-off-by: Peter Lieven <pl@kamp.de>
> > ---
> > block/iscsi.c | 7 +++++++
> > 1 file changed, 7 insertions(+)
> >
> > diff --git a/block/iscsi.c b/block/iscsi.c
> > index 2ca8e72..e7d5f7b 100644
> > --- a/block/iscsi.c
> > +++ b/block/iscsi.c
> > @@ -833,6 +833,13 @@ static BlockAIOCB *iscsi_aio_ioctl(BlockDriverState *bs,
> > return &acb->common;
> > }
> >
> > + if (acb->ioh->cmd_len > SCSI_CDB_MAX_SIZE) {
> > + error_report("iSCSI: ioctl error CDB exceeds max size (%d > %d)",
> > + acb->ioh->cmd_len, SCSI_CDB_MAX_SIZE);
> > + qemu_aio_unref(acb);
> > + return NULL;
> > + }
> > +
> > acb->task = malloc(sizeof(struct scsi_task));
> > if (acb->task == NULL) {
> > error_report("iSCSI: Failed to allocate task for scsi command. %s",
>
> Is it better to invoke the cb and report -EINVAL to the caller?
You need to implement the BH manually then. The difference is -EINVAL
vs. -ENOTSUP, which don't result in a different guest behaviour. So I
think returning NULL is simpler and therefore better.
Kevin
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Qemu-devel] [Qemu-block] [PATCH] block/iscsi: avoid potential overflow of acb->task->cdb
2016-05-31 7:35 ` [Qemu-devel] [Qemu-block] " Kevin Wolf
@ 2016-05-31 8:01 ` Fam Zheng
0 siblings, 0 replies; 6+ messages in thread
From: Fam Zheng @ 2016-05-31 8:01 UTC (permalink / raw)
To: Kevin Wolf; +Cc: Peter Lieven, pbonzini, qemu-devel, qemu-block, qemu-stable
On Tue, 05/31 09:35, Kevin Wolf wrote:
> Am 31.05.2016 um 08:44 hat Fam Zheng geschrieben:
> > On Tue, 05/24 10:59, Peter Lieven wrote:
> > > at least in the path via virtio-blk the maximum size is not
> > > restricted.
> > >
> > > Cc: qemu-stable@nongnu.org
> > > Signed-off-by: Peter Lieven <pl@kamp.de>
> > > ---
> > > block/iscsi.c | 7 +++++++
> > > 1 file changed, 7 insertions(+)
> > >
> > > diff --git a/block/iscsi.c b/block/iscsi.c
> > > index 2ca8e72..e7d5f7b 100644
> > > --- a/block/iscsi.c
> > > +++ b/block/iscsi.c
> > > @@ -833,6 +833,13 @@ static BlockAIOCB *iscsi_aio_ioctl(BlockDriverState *bs,
> > > return &acb->common;
> > > }
> > >
> > > + if (acb->ioh->cmd_len > SCSI_CDB_MAX_SIZE) {
> > > + error_report("iSCSI: ioctl error CDB exceeds max size (%d > %d)",
> > > + acb->ioh->cmd_len, SCSI_CDB_MAX_SIZE);
> > > + qemu_aio_unref(acb);
> > > + return NULL;
> > > + }
> > > +
> > > acb->task = malloc(sizeof(struct scsi_task));
> > > if (acb->task == NULL) {
> > > error_report("iSCSI: Failed to allocate task for scsi command. %s",
> >
> > Is it better to invoke the cb and report -EINVAL to the caller?
>
> You need to implement the BH manually then. The difference is -EINVAL
> vs. -ENOTSUP, which don't result in a different guest behaviour. So I
> think returning NULL is simpler and therefore better.
Makes sense. Thanks for explaining!
Reviewed-by: Fam Zheng <famz@redhat.com>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [Qemu-block] [PATCH] block/iscsi: avoid potential overflow of acb->task->cdb
2016-05-31 6:44 ` Fam Zheng
2016-05-31 7:35 ` [Qemu-devel] [Qemu-block] " Kevin Wolf
@ 2016-05-31 7:39 ` Kevin Wolf
2016-05-31 8:03 ` Fam Zheng
1 sibling, 1 reply; 6+ messages in thread
From: Kevin Wolf @ 2016-05-31 7:39 UTC (permalink / raw)
To: Fam Zheng; +Cc: Peter Lieven, pbonzini, qemu-devel, qemu-block, qemu-stable
Am 31.05.2016 um 08:44 hat Fam Zheng geschrieben:
> On Tue, 05/24 10:59, Peter Lieven wrote:
> > at least in the path via virtio-blk the maximum size is not
> > restricted.
> >
> > Cc: qemu-stable@nongnu.org
> > Signed-off-by: Peter Lieven <pl@kamp.de>
> > ---
> > block/iscsi.c | 7 +++++++
> > 1 file changed, 7 insertions(+)
> >
> > diff --git a/block/iscsi.c b/block/iscsi.c
> > index 2ca8e72..e7d5f7b 100644
> > --- a/block/iscsi.c
> > +++ b/block/iscsi.c
> > @@ -833,6 +833,13 @@ static BlockAIOCB *iscsi_aio_ioctl(BlockDriverState *bs,
> > return &acb->common;
> > }
> >
> > + if (acb->ioh->cmd_len > SCSI_CDB_MAX_SIZE) {
> > + error_report("iSCSI: ioctl error CDB exceeds max size (%d > %d)",
> > + acb->ioh->cmd_len, SCSI_CDB_MAX_SIZE);
> > + qemu_aio_unref(acb);
> > + return NULL;
> > + }
> > +
> > acb->task = malloc(sizeof(struct scsi_task));
> > if (acb->task == NULL) {
> > error_report("iSCSI: Failed to allocate task for scsi command. %s",
>
> Is it better to invoke the cb and report -EINVAL to the caller?
By the way, when returning NULL, it looks like bdrv_co_do_ioctl()
leaks its BdrvIoctlCompletionData. This code was introduced by your
commit 5c5ae76a ("block: Emulate bdrv_ioctl with bdrv_aio_ioctl and
track both").
Kevin
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Qemu-devel] [Qemu-block] [PATCH] block/iscsi: avoid potential overflow of acb->task->cdb
2016-05-31 7:39 ` Kevin Wolf
@ 2016-05-31 8:03 ` Fam Zheng
0 siblings, 0 replies; 6+ messages in thread
From: Fam Zheng @ 2016-05-31 8:03 UTC (permalink / raw)
To: Kevin Wolf; +Cc: Peter Lieven, pbonzini, qemu-devel, qemu-block, qemu-stable
On Tue, 05/31 09:39, Kevin Wolf wrote:
> Am 31.05.2016 um 08:44 hat Fam Zheng geschrieben:
> > On Tue, 05/24 10:59, Peter Lieven wrote:
> > > at least in the path via virtio-blk the maximum size is not
> > > restricted.
> > >
> > > Cc: qemu-stable@nongnu.org
> > > Signed-off-by: Peter Lieven <pl@kamp.de>
> > > ---
> > > block/iscsi.c | 7 +++++++
> > > 1 file changed, 7 insertions(+)
> > >
> > > diff --git a/block/iscsi.c b/block/iscsi.c
> > > index 2ca8e72..e7d5f7b 100644
> > > --- a/block/iscsi.c
> > > +++ b/block/iscsi.c
> > > @@ -833,6 +833,13 @@ static BlockAIOCB *iscsi_aio_ioctl(BlockDriverState *bs,
> > > return &acb->common;
> > > }
> > >
> > > + if (acb->ioh->cmd_len > SCSI_CDB_MAX_SIZE) {
> > > + error_report("iSCSI: ioctl error CDB exceeds max size (%d > %d)",
> > > + acb->ioh->cmd_len, SCSI_CDB_MAX_SIZE);
> > > + qemu_aio_unref(acb);
> > > + return NULL;
> > > + }
> > > +
> > > acb->task = malloc(sizeof(struct scsi_task));
> > > if (acb->task == NULL) {
> > > error_report("iSCSI: Failed to allocate task for scsi command. %s",
> >
> > Is it better to invoke the cb and report -EINVAL to the caller?
>
> By the way, when returning NULL, it looks like bdrv_co_do_ioctl()
> leaks its BdrvIoctlCompletionData. This code was introduced by your
> commit 5c5ae76a ("block: Emulate bdrv_ioctl with bdrv_aio_ioctl and
> track both").
I'll send a patch to fix it now.
Fam
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-05-31 8:03 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-05-24 8:59 [Qemu-devel] [PATCH] block/iscsi: avoid potential overflow of acb->task->cdb Peter Lieven
2016-05-31 6:44 ` Fam Zheng
2016-05-31 7:35 ` [Qemu-devel] [Qemu-block] " Kevin Wolf
2016-05-31 8:01 ` Fam Zheng
2016-05-31 7:39 ` Kevin Wolf
2016-05-31 8:03 ` Fam Zheng
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).