Re: [Qemu-block] [PATCH v2 1/2] block: vpc - prevent overflow if max_table_entries = 0x40000000

[Max Reitz]

On 22.07.2015 19:26, Jeff Cody wrote:

On Wed, Jul 22, 2015 at 07:02:02PM +0200, Max Reitz wrote:

On 21.07.2015 18:13, Jeff Cody wrote:

When we allocate the pagetable based on max_table_entries, we multiply
the max table entry value by 4 to accomodate a table of 32-bit integers.
However, max_table_entries is a uint32_t, and the VPC driver accepts
ranges for that entry over 0x4000.  So during this allocation:

s-pagetable = qemu_try_blockalign(bs-file, s-max_table_entries * 4);

The size arg overflows, allocating significantly less memory than
expected.

Since qemu_try_blockalign() size argument is size_t, cast the
multiplication correctly to prevent overflow.

The value of max_table_entries * 4 is used elsewhere in the code as
well, so store the correct value for use in all those cases.

We also check the Max Tables Entries value, to make sure that it is 
SIZE_MAX / 4, so we know the pagetable size will fit in size_t.

Reported-by: Richard W.M. Jones rjo...@redhat.com
Signed-off-by: Jeff Cody jc...@redhat.com
---
  block/vpc.c | 17 +
  1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/block/vpc.c b/block/vpc.c
index 37572ba..6ef21e6 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -168,6 +168,7 @@ static int vpc_open(BlockDriverState *bs, QDict *options, 
int flags,
  uint8_t buf[HEADER_SIZE];
  uint32_t checksum;
  uint64_t computed_size;
+uint64_t pagetable_size;
  int disk_type = VHD_DYNAMIC;
  int ret;

@@ -269,7 +270,16 @@ static int vpc_open(BlockDriverState *bs, QDict *options, 
int flags,
  goto fail;
  }

-s-pagetable = qemu_try_blockalign(bs-file, s-max_table_entries * 4);
+if (s-max_table_entries  SIZE_MAX / 4) {
+error_setg(errp, Max Table Entries too large (% PRId32 ),
+s-max_table_entries);
+ret = -EINVAL;
+goto fail;
+}
+
+pagetable_size = (uint64_t) s-max_table_entries * 4;
+
+s-pagetable = qemu_try_blockalign(bs-file, pagetable_size);
  if (s-pagetable == NULL) {
  ret = -ENOMEM;
  goto fail;
@@ -277,14 +287,13 @@ static int vpc_open(BlockDriverState *bs, QDict *options, 
int flags,

  s-bat_offset = be64_to_cpu(dyndisk_header-table_offset);

-ret = bdrv_pread(bs-file, s-bat_offset, s-pagetable,
- s-max_table_entries * 4);
+ret = bdrv_pread(bs-file, s-bat_offset, s-pagetable, 
pagetable_size);


The last parameter of bdrv_pread() is an int, so this will still
overflow if s-max_table_entries  INT_MAX / 4.



I thought about checking for INT_MAX overflow as well - I probably
should.  I wasn't too concerned about it, however, as bdrv_read() will
return -EINVAL for nb_sectors  0.


Right, but SIZE_MAX may be larger than 2ull * INT_MAX, so on 64 bit 
systems, pagetable_size might become large enough for 
(int)pagetable_size to be positive, with (int)pagetable_size != 
pagetable_size, which would make bdrv_pread() work just fine, but only 
read a part of the table.


Max


  if (ret  0) {
  goto fail;
  }

  s-free_data_block_offset =
-(s-bat_offset + (s-max_table_entries * 4) + 511)  ~511;
+(s-bat_offset + pagetable_size + 511)  ~511;


Not necessary, but perhaps we should be using ROUND_UP() here...



Sure, why not :)


Max


  for (i = 0; i  s-max_table_entries; i++) {
  be32_to_cpus(s-pagetable[i]);

Re: [Qemu-block] [PATCH v2 1/2] block: vpc - prevent overflow if max_table_entries = 0x40000000

[Max Reitz]

On 22.07.2015 19:40, Jeff Cody wrote:

On Wed, Jul 22, 2015 at 07:29:47PM +0200, Max Reitz wrote:

On 22.07.2015 19:26, Jeff Cody wrote:

On Wed, Jul 22, 2015 at 07:02:02PM +0200, Max Reitz wrote:

On 21.07.2015 18:13, Jeff Cody wrote:

When we allocate the pagetable based on max_table_entries, we multiply
the max table entry value by 4 to accomodate a table of 32-bit integers.
However, max_table_entries is a uint32_t, and the VPC driver accepts
ranges for that entry over 0x4000.  So during this allocation:

s-pagetable = qemu_try_blockalign(bs-file, s-max_table_entries * 4);

The size arg overflows, allocating significantly less memory than
expected.

Since qemu_try_blockalign() size argument is size_t, cast the
multiplication correctly to prevent overflow.

The value of max_table_entries * 4 is used elsewhere in the code as
well, so store the correct value for use in all those cases.

We also check the Max Tables Entries value, to make sure that it is 
SIZE_MAX / 4, so we know the pagetable size will fit in size_t.

Reported-by: Richard W.M. Jones rjo...@redhat.com
Signed-off-by: Jeff Cody jc...@redhat.com
---
  block/vpc.c | 17 +
  1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/block/vpc.c b/block/vpc.c
index 37572ba..6ef21e6 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -168,6 +168,7 @@ static int vpc_open(BlockDriverState *bs, QDict *options, 
int flags,
  uint8_t buf[HEADER_SIZE];
  uint32_t checksum;
  uint64_t computed_size;
+uint64_t pagetable_size;
  int disk_type = VHD_DYNAMIC;
  int ret;

@@ -269,7 +270,16 @@ static int vpc_open(BlockDriverState *bs, QDict *options, 
int flags,
  goto fail;
  }

-s-pagetable = qemu_try_blockalign(bs-file, s-max_table_entries * 4);
+if (s-max_table_entries  SIZE_MAX / 4) {
+error_setg(errp, Max Table Entries too large (% PRId32 ),
+s-max_table_entries);
+ret = -EINVAL;
+goto fail;
+}
+
+pagetable_size = (uint64_t) s-max_table_entries * 4;
+
+s-pagetable = qemu_try_blockalign(bs-file, pagetable_size);
  if (s-pagetable == NULL) {
  ret = -ENOMEM;
  goto fail;
@@ -277,14 +287,13 @@ static int vpc_open(BlockDriverState *bs, QDict *options, 
int flags,

  s-bat_offset = be64_to_cpu(dyndisk_header-table_offset);

-ret = bdrv_pread(bs-file, s-bat_offset, s-pagetable,
- s-max_table_entries * 4);
+ret = bdrv_pread(bs-file, s-bat_offset, s-pagetable, 
pagetable_size);


The last parameter of bdrv_pread() is an int, so this will still
overflow if s-max_table_entries  INT_MAX / 4.



I thought about checking for INT_MAX overflow as well - I probably
should.  I wasn't too concerned about it, however, as bdrv_read() will
return -EINVAL for nb_sectors  0.


Right, but SIZE_MAX may be larger than 2ull * INT_MAX, so on 64 bit
systems, pagetable_size might become large enough for
(int)pagetable_size to be positive, with (int)pagetable_size !=
pagetable_size, which would make bdrv_pread() work just fine, but
only read a part of the table.


The maximum size pagetable_size can be is UINT32_MAX * 4, because
max_table_entries is a 32-bit value from the VHD header.  But either
way, I'll add a check for INT_MAX, as that really is the correct (and
clearest) way to handle it.


Ah, right. Too obvious for me to see, I guess. *cough*

Max


  if (ret  0) {
  goto fail;
  }

  s-free_data_block_offset =
-(s-bat_offset + (s-max_table_entries * 4) + 511)  ~511;
+(s-bat_offset + pagetable_size + 511)  ~511;


Not necessary, but perhaps we should be using ROUND_UP() here...



Sure, why not :)


Max


  for (i = 0; i  s-max_table_entries; i++) {
  be32_to_cpus(s-pagetable[i]);

Re: [Qemu-block] [PATCH v2 1/2] block: vpc - prevent overflow if max_table_entries = 0x40000000

[Jeff Cody]

On Wed, Jul 22, 2015 at 07:29:47PM +0200, Max Reitz wrote:
 On 22.07.2015 19:26, Jeff Cody wrote:
 On Wed, Jul 22, 2015 at 07:02:02PM +0200, Max Reitz wrote:
 On 21.07.2015 18:13, Jeff Cody wrote:
 When we allocate the pagetable based on max_table_entries, we multiply
 the max table entry value by 4 to accomodate a table of 32-bit integers.
 However, max_table_entries is a uint32_t, and the VPC driver accepts
 ranges for that entry over 0x4000.  So during this allocation:
 
 s-pagetable = qemu_try_blockalign(bs-file, s-max_table_entries * 4);
 
 The size arg overflows, allocating significantly less memory than
 expected.
 
 Since qemu_try_blockalign() size argument is size_t, cast the
 multiplication correctly to prevent overflow.
 
 The value of max_table_entries * 4 is used elsewhere in the code as
 well, so store the correct value for use in all those cases.
 
 We also check the Max Tables Entries value, to make sure that it is 
 SIZE_MAX / 4, so we know the pagetable size will fit in size_t.
 
 Reported-by: Richard W.M. Jones rjo...@redhat.com
 Signed-off-by: Jeff Cody jc...@redhat.com
 ---
   block/vpc.c | 17 +
   1 file changed, 13 insertions(+), 4 deletions(-)
 
 diff --git a/block/vpc.c b/block/vpc.c
 index 37572ba..6ef21e6 100644
 --- a/block/vpc.c
 +++ b/block/vpc.c
 @@ -168,6 +168,7 @@ static int vpc_open(BlockDriverState *bs, QDict 
 *options, int flags,
   uint8_t buf[HEADER_SIZE];
   uint32_t checksum;
   uint64_t computed_size;
 +uint64_t pagetable_size;
   int disk_type = VHD_DYNAMIC;
   int ret;
 
 @@ -269,7 +270,16 @@ static int vpc_open(BlockDriverState *bs, QDict 
 *options, int flags,
   goto fail;
   }
 
 -s-pagetable = qemu_try_blockalign(bs-file, s-max_table_entries 
 * 4);
 +if (s-max_table_entries  SIZE_MAX / 4) {
 +error_setg(errp, Max Table Entries too large (% PRId32 ),
 +s-max_table_entries);
 +ret = -EINVAL;
 +goto fail;
 +}
 +
 +pagetable_size = (uint64_t) s-max_table_entries * 4;
 +
 +s-pagetable = qemu_try_blockalign(bs-file, pagetable_size);
   if (s-pagetable == NULL) {
   ret = -ENOMEM;
   goto fail;
 @@ -277,14 +287,13 @@ static int vpc_open(BlockDriverState *bs, QDict 
 *options, int flags,
 
   s-bat_offset = be64_to_cpu(dyndisk_header-table_offset);
 
 -ret = bdrv_pread(bs-file, s-bat_offset, s-pagetable,
 - s-max_table_entries * 4);
 +ret = bdrv_pread(bs-file, s-bat_offset, s-pagetable, 
 pagetable_size);
 
 The last parameter of bdrv_pread() is an int, so this will still
 overflow if s-max_table_entries  INT_MAX / 4.
 
 
 I thought about checking for INT_MAX overflow as well - I probably
 should.  I wasn't too concerned about it, however, as bdrv_read() will
 return -EINVAL for nb_sectors  0.
 
 Right, but SIZE_MAX may be larger than 2ull * INT_MAX, so on 64 bit
 systems, pagetable_size might become large enough for
 (int)pagetable_size to be positive, with (int)pagetable_size !=
 pagetable_size, which would make bdrv_pread() work just fine, but
 only read a part of the table.

The maximum size pagetable_size can be is UINT32_MAX * 4, because
max_table_entries is a 32-bit value from the VHD header.  But either
way, I'll add a check for INT_MAX, as that really is the correct (and
clearest) way to handle it.

 
 Max
 
   if (ret  0) {
   goto fail;
   }
 
   s-free_data_block_offset =
 -(s-bat_offset + (s-max_table_entries * 4) + 511)  ~511;
 +(s-bat_offset + pagetable_size + 511)  ~511;
 
 Not necessary, but perhaps we should be using ROUND_UP() here...
 
 
 Sure, why not :)
 
 Max
 
   for (i = 0; i  s-max_table_entries; i++) {
   be32_to_cpus(s-pagetable[i]);
 
 
 

Re: [Qemu-block] [PATCH v2 1/2] block: vpc - prevent overflow if max_table_entries = 0x40000000

[Max Reitz]

On 21.07.2015 18:13, Jeff Cody wrote:

When we allocate the pagetable based on max_table_entries, we multiply
the max table entry value by 4 to accomodate a table of 32-bit integers.
However, max_table_entries is a uint32_t, and the VPC driver accepts
ranges for that entry over 0x4000.  So during this allocation:

s-pagetable = qemu_try_blockalign(bs-file, s-max_table_entries * 4);

The size arg overflows, allocating significantly less memory than
expected.

Since qemu_try_blockalign() size argument is size_t, cast the
multiplication correctly to prevent overflow.

The value of max_table_entries * 4 is used elsewhere in the code as
well, so store the correct value for use in all those cases.

We also check the Max Tables Entries value, to make sure that it is 
SIZE_MAX / 4, so we know the pagetable size will fit in size_t.

Reported-by: Richard W.M. Jones rjo...@redhat.com
Signed-off-by: Jeff Cody jc...@redhat.com
---
  block/vpc.c | 17 +
  1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/block/vpc.c b/block/vpc.c
index 37572ba..6ef21e6 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -168,6 +168,7 @@ static int vpc_open(BlockDriverState *bs, QDict *options, 
int flags,
  uint8_t buf[HEADER_SIZE];
  uint32_t checksum;
  uint64_t computed_size;
+uint64_t pagetable_size;
  int disk_type = VHD_DYNAMIC;
  int ret;

@@ -269,7 +270,16 @@ static int vpc_open(BlockDriverState *bs, QDict *options, 
int flags,
  goto fail;
  }

-s-pagetable = qemu_try_blockalign(bs-file, s-max_table_entries * 4);
+if (s-max_table_entries  SIZE_MAX / 4) {
+error_setg(errp, Max Table Entries too large (% PRId32 ),
+s-max_table_entries);
+ret = -EINVAL;
+goto fail;
+}
+
+pagetable_size = (uint64_t) s-max_table_entries * 4;
+
+s-pagetable = qemu_try_blockalign(bs-file, pagetable_size);
  if (s-pagetable == NULL) {
  ret = -ENOMEM;
  goto fail;
@@ -277,14 +287,13 @@ static int vpc_open(BlockDriverState *bs, QDict *options, 
int flags,

  s-bat_offset = be64_to_cpu(dyndisk_header-table_offset);

-ret = bdrv_pread(bs-file, s-bat_offset, s-pagetable,
- s-max_table_entries * 4);
+ret = bdrv_pread(bs-file, s-bat_offset, s-pagetable, 
pagetable_size);


The last parameter of bdrv_pread() is an int, so this will still 
overflow if s-max_table_entries  INT_MAX / 4.



  if (ret  0) {
  goto fail;
  }

  s-free_data_block_offset =
-(s-bat_offset + (s-max_table_entries * 4) + 511)  ~511;
+(s-bat_offset + pagetable_size + 511)  ~511;


Not necessary, but perhaps we should be using ROUND_UP() here...

Max


  for (i = 0; i  s-max_table_entries; i++) {
  be32_to_cpus(s-pagetable[i]);

Re: [Qemu-block] [PATCH v2 1/2] block: vpc - prevent overflow if max_table_entries = 0x40000000

[Jeff Cody]

On Wed, Jul 22, 2015 at 07:02:02PM +0200, Max Reitz wrote:
 On 21.07.2015 18:13, Jeff Cody wrote:
 When we allocate the pagetable based on max_table_entries, we multiply
 the max table entry value by 4 to accomodate a table of 32-bit integers.
 However, max_table_entries is a uint32_t, and the VPC driver accepts
 ranges for that entry over 0x4000.  So during this allocation:
 
 s-pagetable = qemu_try_blockalign(bs-file, s-max_table_entries * 4);
 
 The size arg overflows, allocating significantly less memory than
 expected.
 
 Since qemu_try_blockalign() size argument is size_t, cast the
 multiplication correctly to prevent overflow.
 
 The value of max_table_entries * 4 is used elsewhere in the code as
 well, so store the correct value for use in all those cases.
 
 We also check the Max Tables Entries value, to make sure that it is 
 SIZE_MAX / 4, so we know the pagetable size will fit in size_t.
 
 Reported-by: Richard W.M. Jones rjo...@redhat.com
 Signed-off-by: Jeff Cody jc...@redhat.com
 ---
   block/vpc.c | 17 +
   1 file changed, 13 insertions(+), 4 deletions(-)
 
 diff --git a/block/vpc.c b/block/vpc.c
 index 37572ba..6ef21e6 100644
 --- a/block/vpc.c
 +++ b/block/vpc.c
 @@ -168,6 +168,7 @@ static int vpc_open(BlockDriverState *bs, QDict 
 *options, int flags,
   uint8_t buf[HEADER_SIZE];
   uint32_t checksum;
   uint64_t computed_size;
 +uint64_t pagetable_size;
   int disk_type = VHD_DYNAMIC;
   int ret;
 
 @@ -269,7 +270,16 @@ static int vpc_open(BlockDriverState *bs, QDict 
 *options, int flags,
   goto fail;
   }
 
 -s-pagetable = qemu_try_blockalign(bs-file, s-max_table_entries * 
 4);
 +if (s-max_table_entries  SIZE_MAX / 4) {
 +error_setg(errp, Max Table Entries too large (% PRId32 ),
 +s-max_table_entries);
 +ret = -EINVAL;
 +goto fail;
 +}
 +
 +pagetable_size = (uint64_t) s-max_table_entries * 4;
 +
 +s-pagetable = qemu_try_blockalign(bs-file, pagetable_size);
   if (s-pagetable == NULL) {
   ret = -ENOMEM;
   goto fail;
 @@ -277,14 +287,13 @@ static int vpc_open(BlockDriverState *bs, QDict 
 *options, int flags,
 
   s-bat_offset = be64_to_cpu(dyndisk_header-table_offset);
 
 -ret = bdrv_pread(bs-file, s-bat_offset, s-pagetable,
 - s-max_table_entries * 4);
 +ret = bdrv_pread(bs-file, s-bat_offset, s-pagetable, 
 pagetable_size);
 
 The last parameter of bdrv_pread() is an int, so this will still
 overflow if s-max_table_entries  INT_MAX / 4.


I thought about checking for INT_MAX overflow as well - I probably
should.  I wasn't too concerned about it, however, as bdrv_read() will
return -EINVAL for nb_sectors  0.


   if (ret  0) {
   goto fail;
   }
 
   s-free_data_block_offset =
 -(s-bat_offset + (s-max_table_entries * 4) + 511)  ~511;
 +(s-bat_offset + pagetable_size + 511)  ~511;
 
 Not necessary, but perhaps we should be using ROUND_UP() here...
 

Sure, why not :)

 Max
 
   for (i = 0; i  s-max_table_entries; i++) {
   be32_to_cpus(s-pagetable[i]);
 
 

[Qemu-block] [PATCH V3 3/3] virtio-blk: set VIRTIO_F_ANY_LAYOUT when 1.0 is supported

[Jason Wang]

Chapter 6.3 of spec said


Transitional devices MUST offer, and if offered by the device
transitional drivers MUST accept the following:

VIRTIO_F_ANY_LAYOUT (27)


So this patch sets VIRTIO_F_ANY_LAYOUT when 1.0 is supported.

Cc: Stefan Hajnoczi stefa...@redhat.com
Cc: Kevin Wolf kw...@redhat.com
Cc: qemu-block@nongnu.org
Signed-off-by: Jason Wang jasow...@redhat.com
---
 hw/block/virtio-blk.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 4716c3e..2e7a190 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -736,6 +736,7 @@ static uint64_t virtio_blk_get_features(VirtIODevice *vdev, 
uint64_t features,
 error_setg(errp, Virtio 1.0 does not support scsi passthrough!);
 return 0;
 }
+virtio_add_feature(features, VIRTIO_F_ANY_LAYOUT);
 } else {
 virtio_add_feature(features, VIRTIO_BLK_F_SCSI);
 }
-- 
2.1.4

[Qemu-block] [PATCH RFC] virtio: set any_layout in virtio core

[Michael S. Tsirkin]

Virtio 1 requires this, and all devices are clean by now,
so let's do it!

Exceptions:
- virtio-blk
- compat machine types

Signed-off-by: Michael S. Tsirkin m...@redhat.com
---

Untested - consider this pseudo-code - it just seems easier to write it
in C than try to explain it.

 include/hw/compat.h| 22 +-
 include/hw/virtio/virtio.h |  4 +++-
 hw/block/virtio-blk.c  |  1 +
 hw/net/virtio-net.c|  2 --
 hw/scsi/virtio-scsi.c  |  2 --
 5 files changed, 25 insertions(+), 6 deletions(-)

diff --git a/include/hw/compat.h b/include/hw/compat.h
index 4a43466..94c8097 100644
--- a/include/hw/compat.h
+++ b/include/hw/compat.h
@@ -2,7 +2,27 @@
 #define HW_COMPAT_H
 
 #define HW_COMPAT_2_3 \
-/* empty */
+{\
+.driver   = virtio-blk-pci,\
+.property = any_layout,\
+.value= off,\
+},{\
+.driver   = virtio-balloon-pci,\
+.property = any_layout,\
+.value= off,\
+},{\
+.driver   = virtio-serial-pci,\
+.property = any_layout,\
+.value= off,\
+},{\
+.driver   = virtio-9p-pci,\
+.property = any_layout,\
+.value= off,\
+},{\
+.driver   = virtio-rng-pci,\
+.property = any_layout,\
+.value= off,\
+},
 
 #define HW_COMPAT_2_2 \
 /* empty */
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index 473fb75..fbb3c06 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -214,7 +214,9 @@ typedef struct VirtIORNGConf VirtIORNGConf;
 DEFINE_PROP_BIT64(event_idx, _state, _field,\
   VIRTIO_RING_F_EVENT_IDX, true), \
 DEFINE_PROP_BIT64(notify_on_empty, _state, _field,  \
-  VIRTIO_F_NOTIFY_ON_EMPTY, true)
+  VIRTIO_F_NOTIFY_ON_EMPTY, true) \
+DEFINE_PROP_BIT64(any_layout, _state, _field,  \
+  VIRTIO_F_ANY_LAYOUT, true)
 
 hwaddr virtio_queue_get_desc_addr(VirtIODevice *vdev, int n);
 hwaddr virtio_queue_get_avail_addr(VirtIODevice *vdev, int n);
diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 6aefda4..015b9b5 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -731,6 +731,7 @@ static uint64_t virtio_blk_get_features(VirtIODevice *vdev, 
uint64_t features)
 virtio_add_feature(features, VIRTIO_BLK_F_TOPOLOGY);
 virtio_add_feature(features, VIRTIO_BLK_F_BLK_SIZE);
 virtio_add_feature(features, VIRTIO_BLK_F_SCSI);
+virtio_clear_feature(features, VIRTIO_F_ANY_LAYOUT);
 
 if (s-conf.config_wce) {
 virtio_add_feature(features, VIRTIO_BLK_F_CONFIG_WCE);
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 304d3dd..e203058 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -1777,8 +1777,6 @@ static void virtio_net_instance_init(Object *obj)
 }
 
 static Property virtio_net_properties[] = {
-DEFINE_PROP_BIT(any_layout, VirtIONet, host_features,
-VIRTIO_F_ANY_LAYOUT, true),
 DEFINE_PROP_BIT(csum, VirtIONet, host_features, VIRTIO_NET_F_CSUM, true),
 DEFINE_PROP_BIT(guest_csum, VirtIONet, host_features,
 VIRTIO_NET_F_GUEST_CSUM, true),
diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
index f7d3c7c..d17698d 100644
--- a/hw/scsi/virtio-scsi.c
+++ b/hw/scsi/virtio-scsi.c
@@ -953,8 +953,6 @@ static Property virtio_scsi_properties[] = {
   0x),
 DEFINE_PROP_UINT32(cmd_per_lun, VirtIOSCSI, parent_obj.conf.cmd_per_lun,
   128),
-DEFINE_PROP_BIT(any_layout, VirtIOSCSI, host_features,
-  VIRTIO_F_ANY_LAYOUT, true),
 DEFINE_PROP_BIT(hotplug, VirtIOSCSI, host_features,
VIRTIO_SCSI_F_HOTPLUG, true),
 DEFINE_PROP_BIT(param_change, VirtIOSCSI, host_features,
-- 
MST

[Qemu-block] [PATCH] block/ssh: Avoid segfault if inet_connect doesn't set errno.

[Richard W.M. Jones]

On some (but not all) systems:

  $ qemu-img create -f qcow2 overlay -b ssh://xen/
  Segmentation fault

It turns out this happens when inet_connect returns NULL in the
following code, but errno is not set (0).

  s-sock = inet_connect(s-hostport, errp);
  if (s-sock  0) {
  ret = -errno;
  goto err;
  }

In the case above, 'xen' doesn't exist so getaddrinfo fails.  On
Fedora 22, getaddrinfo happens to set errno = ENOENT (although it is
_not_ documented to do that), so it doesn't segfault.

On RHEL 7, errno is not set by the failing getaddrinfo, so ret == 0,
so the caller doesn't know there was an error and continues with a
half-initialized BDRVSSHState struct, and everything goes south from
there.

Fix this by setting errno to EINVAL.  The real error is saved in the
Error** errp struct, so it is printed correctly:

  $ ./qemu-img create -f qcow2 overlay -b ssh://xen/
  qemu-img: overlay: address resolution failed for xen:22: No address 
associated with hostname

BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1147343
Signed-off-by: Richard W.M. Jones rjo...@redhat.com
---
 block/ssh.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/ssh.c b/block/ssh.c
index aebb18c..8d4dc2a 100644
--- a/block/ssh.c
+++ b/block/ssh.c
@@ -563,7 +563,7 @@ static int connect_to_ssh(BDRVSSHState *s, QDict *options,
 /* Open the socket and connect. */
 s-sock = inet_connect(s-hostport, errp);
 if (s-sock  0) {
-ret = -errno;
+ret = -EINVAL;
 goto err;
 }
 
-- 
2.4.3

Re: [Qemu-block] [Qemu-devel] [PATCH v2] block/ssh: Avoid segfault if inet_connect doesn't set errno.

[Daniel P. Berrange]

On Wed, Jul 22, 2015 at 02:07:22PM +0100, Richard W.M. Jones wrote:
 On some (but not all) systems:
 
   $ qemu-img create -f qcow2 overlay -b ssh://xen/
   Segmentation fault
 
 It turns out this happens when inet_connect returns -1 in the
 following code, but errno == 0.
 
   s-sock = inet_connect(s-hostport, errp);
   if (s-sock  0) {
   ret = -errno;
   goto err;
   }
 
 In the test case above, no host called xen exists, so getaddrinfo fails.
 
 On Fedora 22, getaddrinfo happens to set errno = ENOENT (although it
 is *not* documented to do that), so it doesn't segfault.
 
 On RHEL 7, errno is not set by the failing getaddrinfo, so ret =
 -errno = 0, so the caller doesn't know there was an error and
 continues with a half-initialized BDRVSSHState struct, and everything
 goes south from there, eventually resulting in a segfault.
 
 Fix this by setting ret to -EINVAL.  The real error is saved in the
 Error** errp struct, so it is printed correctly:
 
   $ ./qemu-img create -f qcow2 overlay -b ssh://xen/
   qemu-img: overlay: address resolution failed for xen:22: No address 
 associated with hostname
 
 Signed-off-by: Richard W.M. Jones rjo...@redhat.com
 Reported-by: Jun Li
 BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1147343
 ---
  block/ssh.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
 
 diff --git a/block/ssh.c b/block/ssh.c
 index aebb18c..8d4dc2a 100644
 --- a/block/ssh.c
 +++ b/block/ssh.c
 @@ -563,7 +563,7 @@ static int connect_to_ssh(BDRVSSHState *s, QDict *options,
  /* Open the socket and connect. */
  s-sock = inet_connect(s-hostport, errp);
  if (s-sock  0) {
 -ret = -errno;
 +ret = -EINVAL;
  goto err;

There are a reasonable number of other uses of inet_connect() in QEMU,
so can't we fix inet_connect() itself to set EINVAL in the error case
instead of just fixing one caller.


Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|

Re: [Qemu-block] [PATCH v2] block/ssh: Avoid segfault if inet_connect doesn't set errno.

[Jeff Cody]

On Wed, Jul 22, 2015 at 02:07:22PM +0100, Richard W.M. Jones wrote:
 On some (but not all) systems:
 
   $ qemu-img create -f qcow2 overlay -b ssh://xen/
   Segmentation fault
 
 It turns out this happens when inet_connect returns -1 in the
 following code, but errno == 0.
 
   s-sock = inet_connect(s-hostport, errp);
   if (s-sock  0) {
   ret = -errno;
   goto err;
   }
 
 In the test case above, no host called xen exists, so getaddrinfo fails.
 
 On Fedora 22, getaddrinfo happens to set errno = ENOENT (although it
 is *not* documented to do that), so it doesn't segfault.
 
 On RHEL 7, errno is not set by the failing getaddrinfo, so ret =
 -errno = 0, so the caller doesn't know there was an error and
 continues with a half-initialized BDRVSSHState struct, and everything
 goes south from there, eventually resulting in a segfault.
 
 Fix this by setting ret to -EINVAL.  The real error is saved in the
 Error** errp struct, so it is printed correctly:
 
   $ ./qemu-img create -f qcow2 overlay -b ssh://xen/
   qemu-img: overlay: address resolution failed for xen:22: No address 
 associated with hostname
 
 Signed-off-by: Richard W.M. Jones rjo...@redhat.com
 Reported-by: Jun Li
 BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1147343
 ---
  block/ssh.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
 
 diff --git a/block/ssh.c b/block/ssh.c
 index aebb18c..8d4dc2a 100644
 --- a/block/ssh.c
 +++ b/block/ssh.c
 @@ -563,7 +563,7 @@ static int connect_to_ssh(BDRVSSHState *s, QDict *options,
  /* Open the socket and connect. */
  s-sock = inet_connect(s-hostport, errp);
  if (s-sock  0) {
 -ret = -errno;
 +ret = -EINVAL;

Both nbd and sheepdog handle it in a similar fashion (i.e. not relying
on errno being set on inet_connect failure).  However, both nbd and
sheepdog use -EIO as the error return, and I think that makes more
sense.  What do you think?

  goto err;
  }
  
 -- 
 2.4.3
 

Re: [Qemu-block] [Qemu-devel] [PATCH v2] block/ssh: Avoid segfault if inet_connect doesn't set errno.

[Richard W.M. Jones]

On Wed, Jul 22, 2015 at 02:10:51PM +0100, Daniel P. Berrange wrote:
 There are a reasonable number of other uses of inet_connect() in QEMU,
 so can't we fix inet_connect() itself to set EINVAL in the error case
 instead of just fixing one caller.

The only users I can find are block/nbd.c and block/sheepdog.c, and
neither of those is expecting errno to be set.  So I think my use of
errno in block/ssh.c was flat out wrong, although it happened to work
most of the time.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top

Re: [Qemu-block] [Qemu-devel] [PATCH v2] block/ssh: Avoid segfault if inet_connect doesn't set errno.

[Richard W.M. Jones]

On Wed, Jul 22, 2015 at 09:17:49AM -0400, Jeff Cody wrote:
 Both nbd and sheepdog handle it in a similar fashion (i.e. not relying
 on errno being set on inet_connect failure).  However, both nbd and
 sheepdog use -EIO as the error return, and I think that makes more
 sense.  What do you think?

If you prefer -- I don't mind as long as it doesn't segfault :-)

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org

Re: [Qemu-block] [Qemu-devel] [PATCH v2] block/ssh: Avoid segfault if inet_connect doesn't set errno.

[Kevin Wolf]

Am 22.07.2015 um 15:10 hat Daniel P. Berrange geschrieben:
 On Wed, Jul 22, 2015 at 02:07:22PM +0100, Richard W.M. Jones wrote:
  On some (but not all) systems:
  
$ qemu-img create -f qcow2 overlay -b ssh://xen/
Segmentation fault
  
  It turns out this happens when inet_connect returns -1 in the
  following code, but errno == 0.
  
s-sock = inet_connect(s-hostport, errp);
if (s-sock  0) {
ret = -errno;
goto err;
}
  
  In the test case above, no host called xen exists, so getaddrinfo fails.
  
  On Fedora 22, getaddrinfo happens to set errno = ENOENT (although it
  is *not* documented to do that), so it doesn't segfault.
  
  On RHEL 7, errno is not set by the failing getaddrinfo, so ret =
  -errno = 0, so the caller doesn't know there was an error and
  continues with a half-initialized BDRVSSHState struct, and everything
  goes south from there, eventually resulting in a segfault.
  
  Fix this by setting ret to -EINVAL.  The real error is saved in the
  Error** errp struct, so it is printed correctly:
  
$ ./qemu-img create -f qcow2 overlay -b ssh://xen/
qemu-img: overlay: address resolution failed for xen:22: No address 
  associated with hostname
  
  Signed-off-by: Richard W.M. Jones rjo...@redhat.com
  Reported-by: Jun Li
  BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1147343
  ---
   block/ssh.c | 2 +-
   1 file changed, 1 insertion(+), 1 deletion(-)
  
  diff --git a/block/ssh.c b/block/ssh.c
  index aebb18c..8d4dc2a 100644
  --- a/block/ssh.c
  +++ b/block/ssh.c
  @@ -563,7 +563,7 @@ static int connect_to_ssh(BDRVSSHState *s, QDict 
  *options,
   /* Open the socket and connect. */
   s-sock = inet_connect(s-hostport, errp);
   if (s-sock  0) {
  -ret = -errno;
  +ret = -EINVAL;
   goto err;
 
 There are a reasonable number of other uses of inet_connect() in QEMU,
 so can't we fix inet_connect() itself to set EINVAL in the error case
 instead of just fixing one caller.

None of the other callers try to use errno, which isn't even documented
as part of the inet_connect() interface. So I think setting errno inside
inet_connect() would be wrong.

If we were to change anything, we might consider returning -EINVAL
instead of -1, though I'm not sure how useful that change would be. We
would still have to fix this call in the ssh block driver.

Kevin

Re: [Qemu-block] [Qemu-devel] [PATCH v2] block/ssh: Avoid segfault if inet_connect doesn't set errno.

[Jeff Cody]

On Wed, Jul 22, 2015 at 02:22:18PM +0100, Richard W.M. Jones wrote:
 On Wed, Jul 22, 2015 at 09:17:49AM -0400, Jeff Cody wrote:
  Both nbd and sheepdog handle it in a similar fashion (i.e. not relying
  on errno being set on inet_connect failure).  However, both nbd and
  sheepdog use -EIO as the error return, and I think that makes more
  sense.  What do you think?
 
 If you prefer -- I don't mind as long as it doesn't segfault :-)


I think I would - mainly for consistency, but also because -EIO better
describes all the various failures that may occur in inet_connect().

Thanks!
Jeff

[Qemu-block] [PATCH v3] block/ssh: Avoid segfault if inet_connect doesn't set errno.

[Richard W.M. Jones]

v3:
Same as v2 but set the return value to -EIO.

[Qemu-block] [PATCH v3] block/ssh: Avoid segfault if inet_connect doesn't set errno.

[Richard W.M. Jones]

On some (but not all) systems:

  $ qemu-img create -f qcow2 overlay -b ssh://xen/
  Segmentation fault

It turns out this happens when inet_connect returns -1 in the
following code, but errno == 0.

  s-sock = inet_connect(s-hostport, errp);
  if (s-sock  0) {
  ret = -errno;
  goto err;
  }

In the test case above, no host called xen exists, so getaddrinfo fails.

On Fedora 22, getaddrinfo happens to set errno = ENOENT (although it
is *not* documented to do that), so it doesn't segfault.

On RHEL 7, errno is not set by the failing getaddrinfo, so ret =
-errno = 0, so the caller doesn't know there was an error and
continues with a half-initialized BDRVSSHState struct, and everything
goes south from there, eventually resulting in a segfault.

Fix this by setting ret to -EIO (same as block/nbd.c and
block/sheepdog.c).  The real error is saved in the Error** errp
struct, so it is printed correctly:

  $ ./qemu-img create -f qcow2 overlay -b ssh://xen/
  qemu-img: overlay: address resolution failed for xen:22: No address 
associated with hostname

Signed-off-by: Richard W.M. Jones rjo...@redhat.com
Reported-by: Jun Li
BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1147343
---
 block/ssh.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/ssh.c b/block/ssh.c
index aebb18c..8d06739 100644
--- a/block/ssh.c
+++ b/block/ssh.c
@@ -563,7 +563,7 @@ static int connect_to_ssh(BDRVSSHState *s, QDict *options,
 /* Open the socket and connect. */
 s-sock = inet_connect(s-hostport, errp);
 if (s-sock  0) {
-ret = -errno;
+ret = -EIO;
 goto err;
 }
 
-- 
2.4.3

[Qemu-block] [PATCH v2] block/ssh: Avoid segfault if inet_connect doesn't set errno.

[Richard W.M. Jones]

v2:

I fixed several mistakes in the commit message.  The code change
is the same as before.

Rich.

[Qemu-block] [PATCH v2] block/ssh: Avoid segfault if inet_connect doesn't set errno.

[Richard W.M. Jones]

On some (but not all) systems:

  $ qemu-img create -f qcow2 overlay -b ssh://xen/
  Segmentation fault

It turns out this happens when inet_connect returns -1 in the
following code, but errno == 0.

  s-sock = inet_connect(s-hostport, errp);
  if (s-sock  0) {
  ret = -errno;
  goto err;
  }

In the test case above, no host called xen exists, so getaddrinfo fails.

On Fedora 22, getaddrinfo happens to set errno = ENOENT (although it
is *not* documented to do that), so it doesn't segfault.

On RHEL 7, errno is not set by the failing getaddrinfo, so ret =
-errno = 0, so the caller doesn't know there was an error and
continues with a half-initialized BDRVSSHState struct, and everything
goes south from there, eventually resulting in a segfault.

Fix this by setting ret to -EINVAL.  The real error is saved in the
Error** errp struct, so it is printed correctly:

  $ ./qemu-img create -f qcow2 overlay -b ssh://xen/
  qemu-img: overlay: address resolution failed for xen:22: No address 
associated with hostname

Signed-off-by: Richard W.M. Jones rjo...@redhat.com
Reported-by: Jun Li
BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1147343
---
 block/ssh.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/ssh.c b/block/ssh.c
index aebb18c..8d4dc2a 100644
--- a/block/ssh.c
+++ b/block/ssh.c
@@ -563,7 +563,7 @@ static int connect_to_ssh(BDRVSSHState *s, QDict *options,
 /* Open the socket and connect. */
 s-sock = inet_connect(s-hostport, errp);
 if (s-sock  0) {
-ret = -errno;
+ret = -EINVAL;
 goto err;
 }
 
-- 
2.4.3