Re: [PATCH v3 5/5] tests/fp/fp-test: Replace the word 'blacklist'

2021-03-03 Thread Thomas Huth 

On 03/03/2021 19.46, Philippe Mathieu-Daudé wrote:

Follow the inclusive terminology from the "Conscious Language in your
Open Source Projects" guidelines [*] and replace the word "blacklist"
appropriately.

[*] https://github.com/conscious-lang/conscious-lang-docs/blob/main/faq.md

Acked-by: Alex Bennée 
Reviewed-by: Daniel P. Berrangé 
Signed-off-by: Philippe Mathieu-Daudé 
---
  tests/fp/fp-test.c | 8 
  1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/tests/fp/fp-test.c b/tests/fp/fp-test.c
index 06ffebd6db1..5a4cad8c8b2 100644
--- a/tests/fp/fp-test.c
+++ b/tests/fp/fp-test.c
@@ -123,7 +123,7 @@ static void not_implemented(void)
  fprintf(stderr, "Not implemented.\n");
  }
  
-static bool blacklisted(unsigned op, int rmode)

+static bool is_allowed(unsigned op, int rmode)
  {
  /* odd has not been implemented for any 80-bit ops */
  if (rmode == softfloat_round_odd) {
@@ -161,10 +161,10 @@ static bool blacklisted(unsigned op, int rmode)
  case F32_TO_EXTF80:
  case F64_TO_EXTF80:
  case F128_TO_EXTF80:
-return true;
+return false;
  }
  }
-return false;
+return true;
  }
  
  static void do_testfloat(int op, int rmode, bool exact)

@@ -194,7 +194,7 @@ static void do_testfloat(int op, int rmode, bool exact)
  verCases_writeFunctionName(stderr);
  fputs("\n", stderr);
  
-if (blacklisted(op, rmode)) {

+if (!is_allowed(op, rmode)) {
  not_implemented();
  return;
  }



Reviewed-by: Thomas Huth

Re: [PATCH v3 4/5] qemu-options: Replace the word 'blacklist'

2021-03-03 Thread Thomas Huth 

On 03/03/2021 19.46, Philippe Mathieu-Daudé wrote:

Follow the inclusive terminology from the "Conscious Language in your
Open Source Projects" guidelines [*] and replace the word "blacklist"
appropriately.

[*] https://github.com/conscious-lang/conscious-lang-docs/blob/main/faq.md

Reviewed-by: Daniel P. Berrangé 
Signed-off-by: Philippe Mathieu-Daudé 
---
  qemu-options.hx | 6 +++---
  1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/qemu-options.hx b/qemu-options.hx
index 252db9357ca..8462dc5f158 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -4299,12 +4299,12 @@ DEF("sandbox", HAS_ARG, QEMU_OPTION_sandbox, \
  "use 'obsolete' to allow obsolete system calls that are 
provided\n" \
  "by the kernel, but typically no longer used by 
modern\n" \
  "C library implementations.\n" \
-"use 'elevateprivileges' to allow or deny QEMU process to 
elevate\n" \
-"its privileges by blacklisting all set*uid|gid system 
calls.\n" \
+"use 'elevateprivileges' to allow or deny the QEMU process 
ability\n" \
+"to elevate privileges using set*uid|gid system 
calls.\n" \
  "The value 'children' will deny set*uid|gid system calls 
for\n" \
  "main QEMU process but will allow forks and execves to run 
unprivileged\n" \
  "use 'spawn' to avoid QEMU to spawn new threads or processes 
by\n" \
-" blacklisting *fork and execve\n" \
+" blocking *fork and execve\n" \
  "use 'resourcecontrol' to disable process affinity and 
schedular priority\n",
  QEMU_ARCH_ALL)
  SRST



Reviewed-by: Thomas Huth

Re: [PATCH v3 3/5] seccomp: Replace the word 'blacklist'

2021-03-03 Thread Thomas Huth 

On 03/03/2021 19.46, Philippe Mathieu-Daudé wrote:

Follow the inclusive terminology from the "Conscious Language in your
Open Source Projects" guidelines [*] and replace the word "blacklist"
appropriately.

[*] https://github.com/conscious-lang/conscious-lang-docs/blob/main/faq.md

Reviewed-by: Daniel P. Berrangé 
Acked-by: Eduardo Otubo 
Reviewed-by: Alex Bennée 
Signed-off-by: Philippe Mathieu-Daudé 
---
v3: Reworded comment (thuth)
---
  softmmu/qemu-seccomp.c | 16 
  1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/softmmu/qemu-seccomp.c b/softmmu/qemu-seccomp.c
index 377ef6937ca..9c29d9cf007 100644
--- a/softmmu/qemu-seccomp.c
+++ b/softmmu/qemu-seccomp.c
@@ -45,8 +45,8 @@ const struct scmp_arg_cmp sched_setscheduler_arg[] = {
  { .arg = 1, .op = SCMP_CMP_NE, .datum_a = SCHED_IDLE }
  };
  
-static const struct QemuSeccompSyscall blacklist[] = {

-/* default set of syscalls to blacklist */
+static const struct QemuSeccompSyscall denylist[] = {
+/* default set of syscalls that should get blocked */
  { SCMP_SYS(reboot), QEMU_SECCOMP_SET_DEFAULT },
  { SCMP_SYS(swapon), QEMU_SECCOMP_SET_DEFAULT },
  { SCMP_SYS(swapoff),QEMU_SECCOMP_SET_DEFAULT },
@@ -175,18 +175,18 @@ static int seccomp_start(uint32_t seccomp_opts, Error 
**errp)
  goto seccomp_return;
  }
  
-for (i = 0; i < ARRAY_SIZE(blacklist); i++) {

+for (i = 0; i < ARRAY_SIZE(denylist); i++) {
  uint32_t action;
-if (!(seccomp_opts & blacklist[i].set)) {
+if (!(seccomp_opts & denylist[i].set)) {
  continue;
  }
  
-action = qemu_seccomp_get_action(blacklist[i].set);

-rc = seccomp_rule_add_array(ctx, action, blacklist[i].num,
-blacklist[i].narg, blacklist[i].arg_cmp);
+action = qemu_seccomp_get_action(denylist[i].set);
+rc = seccomp_rule_add_array(ctx, action, denylist[i].num,
+denylist[i].narg, denylist[i].arg_cmp);
  if (rc < 0) {
  error_setg_errno(errp, -rc,
- "failed to add seccomp blacklist rules");
+ "failed to add seccomp denylist rules");
  goto seccomp_return;
  }
  }



Reviewed-by: Thomas Huth

Re: [PATCH] file-posix: allow -EBUSY errors during write zeros on block

2021-03-03 Thread ChangLimin 
>> After Linux 5.10, write zeros to a multipath device using
>> ioctl(fd, BLKZEROOUT, range) with cache none or directsync will return EBUSY.
>>
>> Similar to handle_aiocb_write_zeroes_unmap, handle_aiocb_write_zeroes_block
>> allow -EBUSY errors during ioctl(fd, BLKZEROOUT, range).
>>
>> Reference commit in Linux 5.10:
>> https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/?id=384d87ef2c954fc58e6c5fd8253e4a1984f5fe02
> 
>But this can happen only when the block device is used by a file system or
>maybe someone else. In qemu we assume that we are the only user of the
>block device, so EBUSY is a fatal error that should never happen, no?
> 
>Can you explain a real world use case when we get EBUSY?
> 
>Nir
> 

Please refer to 
https://access.redhat.com/documentation/en-us/red_hat_enterprise_linux/8/html/configuring_device_mapper_multipath/index
Where multipath is configured to a SAN LUN, ioctl(fd, BLKZEROOUT, range)
to the /dev/dm-x return EBUSY permanently since Linux 5.10.

ChangLimin

>> Signed-off-by: ChangLimin 
>> ---
>>  block/file-posix.c | 7 ++-
>>  1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/block/file-posix.c b/block/file-posix.c
>> index 05079b40ca..3e60c96214 100644
>> --- a/block/file-posix.c
>> +++ b/block/file-posix.c
>> @@ -1629,8 +1629,13 @@ static ssize_t 
>> handle_aiocb_write_zeroes_block(RawPosixAIOData *aiocb)
>>  } while (errno == EINTR);
>>
>>  ret = translate_err(-errno);
>> -if (ret == -ENOTSUP) {
>> +switch (ret) {
>> +case -ENOTSUP:
>> +case -EINVAL:
>> +case -EBUSY:
>>  s->has_write_zeroes = false;
>> +return -ENOTSUP;
>> +break;
>>  }
>>  }
>>  #endif
>> --
>> 2.27.0
>>

Re: [PATCH] nbd: server: Report holes for raw images

2021-03-03 Thread Eric Blake 
On 3/3/21 3:45 PM, Nir Soffer wrote:


 I'll wait a few days for any other reviewer commentary before taking
 this through my NBD tree.


>>
>> And thanks for CCing me. Hmm, maybe, I'll suggest myself as co-maintainer
>> for NBD?

Vladimir, I'd be happy if you want to do that in a separate patch
(you're already a co-maintainer on block bitmaps, which are somewhat
related).

> 
> 
> Kevin, Max, are you ok with this change?

I guess that means I should send my NBD pull request sooner rather than
later, since it's been a few days with no other comments?

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH] nbd: server: Report holes for raw images

2021-03-03 Thread Nir Soffer 
On Thu, Feb 25, 2021 at 8:51 PM Vladimir Sementsov-Ogievskiy <
vsement...@virtuozzo.com> wrote:

> 19.02.2021 19:58, Eric Blake wrote:
> > On 2/19/21 10:42 AM, Eric Blake wrote:
> >
> >>> To me, data=false looks compatible with NBD_STATE_HOLE. From user point
> >>> of view, getting same results from qemu-nbd and qemu-img is more
> >>> important than being more correct about allocation status.
> >>
> >> More to the point, here is our inconsistency:
> >>
> >> In nbd/server.c, we turn !BDRV_BLOCK_ALLOCATED into NBD_STATE_HOLE
> >>
> >> In block/nbd.c, we turn !NBD_STATE_HOLE into BDRV_BLOCK_DATA
> >>
> >> The fact that we are not doing a round-trip conversion means that one of
> >> the two places is wrong.  And your argument that the server side is
> >> wrong makes sense to me.
> >
> > In fact, when I went back and researched when this was introduced (see
> > commit e7b1948d51 in 2018), we may have been aware of the inconsistency
> > between client and server, but didn't make up our minds at the time:
> > https://lists.gnu.org/archive/html/qemu-devel/2018-03/msg03465.html
> > "? Hm, don't remember, what we decided about DATA/HOLE flags mapping.."
> >
> >>
> >> I'll wait a few days for any other reviewer commentary before taking
> >> this through my NBD tree.
> >>
> >
>
>
> I can add the following.
>
> First, link to my research of block_status in Qemu:
> https://lists.gnu.org/archive/html/qemu-devel/2020-04/msg05136.html
>
> And about HOLE and ZERO..
>
> As I've noted in the research above, SCSI may return HOLE & !ZERO:
>
> from SCSI:
> Logical Block Provisioning Read Zeros (LBPRZ) bit
> 1 If the logical block provisioning read zeros (LBPRZ) bit is set to
> one, then, for an unmapped LBA specified by a read operation, the
> deviceserver shall send user data with all bits set to zero to the data-in
> buffer.
> 0 If the TPRZ bit is set to zero, then, for an unmapped LBA specified
> by a read operation, the device server may send user data with all bitsset
> to any value to the data-in buffer.
>
> So we can have an unmapped area that can be read as any random data. Same
> thing can be said about null-co driver with read-zeroes=false
>
> Also, qcow2 support ALLOCATED ZERO clusters which reads as zero but data
> is allocated - they are reasonable to report as ZERO & !HOLE
>
> And of-course UNALLOCATED ZERO clusters in qcow2 and lseek-holes are
> reasonable to report as ZERO & HOLE,  because they reads as zero and
> "future writes to that area may cause fragmentation or encounter an
> NBD_ENOSPC"..
>
> So, all combination are reasonable, we just need to fix Qemu NBD server to
> report correct statuses in all these cases.
>
> It seems that ZERO/HOLE specification is a lot more reasonable than what
> we have with ZERO/DATA/ALLOCATED in Qemu, and may be true way is move
> internal block_status to use NBD terms.
>
>
> And thanks for CCing me. Hmm, maybe, I'll suggest myself as co-maintainer
> for NBD?


Kevin, Max, are you ok with this change?

Re: QEMU RBD is slow with QCOW2 images

2021-03-03 Thread Peter Lieven 
Am 03.03.21 um 19:47 schrieb Jason Dillaman:
> On Wed, Mar 3, 2021 at 12:41 PM Stefano Garzarella  
> wrote:
>> Hi Jason,
>> as reported in this BZ [1], when qemu-img creates a QCOW2 image on RBD
>> writing data is very slow compared to a raw file.
>>
>> Comparing raw vs QCOW2 image creation with RBD I found that we use a
>> different object size, for the raw file I see '4 MiB objects', for QCOW2
>> I see '64 KiB objects' as reported on comment 14 [2].
>> This should be the main issue of slowness, indeed forcing in the code 4
>> MiB object size also for QCOW2 increased the speed a lot.
>>
>> Looking better I discovered that for raw files, we call rbd_create()
>> with obj_order = 0 (if 'cluster_size' options is not defined), so the
>> default object size is used.
>> Instead for QCOW2, we use obj_order = 16, since the default
>> 'cluster_size' defined for QCOW2, is 64 KiB.
>>
>> Using '-o cluster_size=2M' with qemu-img changed only the qcow2 cluster
>> size, since in qcow2_co_create_opts() we remove the 'cluster_size' from
>> QemuOpts calling qemu_opts_to_qdict_filtered().
>> For some reason that I have yet to understand, after this deletion,
>> however remains in QemuOpts the default value of 'cluster_size' for
>> qcow2 (64 KiB), that it's used in qemu_rbd_co_create_opts()
>>
>> At this point my doubts are:
>> Does it make sense to use the same cluster_size as qcow2 as object_size
>> in RBD?
> No, not really. But it also doesn't really make any sense to put a
> QCOW2 image within an RBD image. To clarify from the BZ, OpenStack
> does not put QCOW2 images on RBD, it converts QCOW2 images into raw
> images to store in RBD.


As discussed earlier the only reasonable format for rbd image is raw.

What is the idea behind putting a qcow2 on an rbd pool?

Jason and I even discussed shortly durign the review of the rbd driver rewrite 
I posted

earlier if it was ok to drop support for writing past the end of file.


Anyway the reason why it is so slow is that write requests serialize if the

qcow2 file grows. If there is a sane reason why we need qcow2 on rbd

we need to implement at least preallocation mode = full to overcome

the serialization.


Peter

Re: [PATCH v2 4/8] scripts/device-crash-test: Replace the word 'whitelist'

2021-03-03 Thread Philippe Mathieu-Daudé 
On 3/3/21 1:06 PM, Alex Bennée wrote:
> 
> Philippe Mathieu-Daudé  writes:
> 
>> Follow the inclusive terminology from the "Conscious Language in your
>> Open Source Projects" guidelines [*] and replace the word "whitelist"
>> appropriately.
>>
>> [*] https://github.com/conscious-lang/conscious-lang-docs/blob/main/faq.md
>>
>> Reviewed-by: Daniel P. Berrangé 
>> Signed-off-by: Philippe Mathieu-Daudé 
>> ---
>>  scripts/device-crash-test | 30 +++---
>>  1 file changed, 15 insertions(+), 15 deletions(-)

>>  def whitelistTestCaseMatch(wl, t):
>> -"""Check if a test case specification can match a whitelist entry
>> +"""Check if a test case specification can match a allowlist entry
> 
> Seems like the function names (and params?) could be updated as well.

Eduardo already fixed/merged this one :)
See 1a14d4e16af ("device-crash-test: Remove problematic language")

[PATCH v3 3/5] seccomp: Replace the word 'blacklist'

2021-03-03 Thread Philippe Mathieu-Daudé 
Follow the inclusive terminology from the "Conscious Language in your
Open Source Projects" guidelines [*] and replace the word "blacklist"
appropriately.

[*] https://github.com/conscious-lang/conscious-lang-docs/blob/main/faq.md

Reviewed-by: Daniel P. Berrangé 
Acked-by: Eduardo Otubo 
Reviewed-by: Alex Bennée 
Signed-off-by: Philippe Mathieu-Daudé 
---
v3: Reworded comment (thuth)
---
 softmmu/qemu-seccomp.c | 16 
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/softmmu/qemu-seccomp.c b/softmmu/qemu-seccomp.c
index 377ef6937ca..9c29d9cf007 100644
--- a/softmmu/qemu-seccomp.c
+++ b/softmmu/qemu-seccomp.c
@@ -45,8 +45,8 @@ const struct scmp_arg_cmp sched_setscheduler_arg[] = {
 { .arg = 1, .op = SCMP_CMP_NE, .datum_a = SCHED_IDLE }
 };
 
-static const struct QemuSeccompSyscall blacklist[] = {
-/* default set of syscalls to blacklist */
+static const struct QemuSeccompSyscall denylist[] = {
+/* default set of syscalls that should get blocked */
 { SCMP_SYS(reboot), QEMU_SECCOMP_SET_DEFAULT },
 { SCMP_SYS(swapon), QEMU_SECCOMP_SET_DEFAULT },
 { SCMP_SYS(swapoff),QEMU_SECCOMP_SET_DEFAULT },
@@ -175,18 +175,18 @@ static int seccomp_start(uint32_t seccomp_opts, Error 
**errp)
 goto seccomp_return;
 }
 
-for (i = 0; i < ARRAY_SIZE(blacklist); i++) {
+for (i = 0; i < ARRAY_SIZE(denylist); i++) {
 uint32_t action;
-if (!(seccomp_opts & blacklist[i].set)) {
+if (!(seccomp_opts & denylist[i].set)) {
 continue;
 }
 
-action = qemu_seccomp_get_action(blacklist[i].set);
-rc = seccomp_rule_add_array(ctx, action, blacklist[i].num,
-blacklist[i].narg, blacklist[i].arg_cmp);
+action = qemu_seccomp_get_action(denylist[i].set);
+rc = seccomp_rule_add_array(ctx, action, denylist[i].num,
+denylist[i].narg, denylist[i].arg_cmp);
 if (rc < 0) {
 error_setg_errno(errp, -rc,
- "failed to add seccomp blacklist rules");
+ "failed to add seccomp denylist rules");
 goto seccomp_return;
 }
 }
-- 
2.26.2

[PATCH v3 0/5] misc: Replace the words 'blacklist/whitelist'

2021-03-03 Thread Philippe Mathieu-Daudé 
Follow the inclusive terminology from the "Conscious Language in your
Open Source Projects" guidelines [*] and replace the words "blacklist"
and "whitelist" appropriately.

Since v2:
- added R-b tags
- reworded seccomp comment (thuth)
- dropped queued vfio patch
- dropped device-crash-test reworked by Eduardo as commit 1a14d4e16af
  ("device-crash-test: Remove problematic language")

Since v1:
- dropped qemu-guest-agent patches
- addressed review comments
- added R-b tags

Series fully reviewed and expected to go via the qemu-trivial@ tree.

[*] https://github.com/conscious-lang/conscious-lang-docs/blob/main/faq.md

Philippe Mathieu-Daudé (5):
  ui: Replace the word 'whitelist'
  scripts/tracetool: Replace the word 'whitelist'
  seccomp: Replace the word 'blacklist'
  qemu-options: Replace the word 'blacklist'
  tests/fp/fp-test: Replace the word 'blacklist'

 softmmu/qemu-seccomp.c| 16 
 tests/fp/fp-test.c|  8 
 ui/console.c  |  2 +-
 ui/vnc-auth-sasl.c|  4 ++--
 qemu-options.hx   |  6 +++---
 scripts/tracetool/__init__.py |  2 +-
 6 files changed, 19 insertions(+), 19 deletions(-)

-- 
2.26.2

[PATCH v3 5/5] tests/fp/fp-test: Replace the word 'blacklist'

2021-03-03 Thread Philippe Mathieu-Daudé 
Follow the inclusive terminology from the "Conscious Language in your
Open Source Projects" guidelines [*] and replace the word "blacklist"
appropriately.

[*] https://github.com/conscious-lang/conscious-lang-docs/blob/main/faq.md

Acked-by: Alex Bennée 
Reviewed-by: Daniel P. Berrangé 
Signed-off-by: Philippe Mathieu-Daudé 
---
 tests/fp/fp-test.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/tests/fp/fp-test.c b/tests/fp/fp-test.c
index 06ffebd6db1..5a4cad8c8b2 100644
--- a/tests/fp/fp-test.c
+++ b/tests/fp/fp-test.c
@@ -123,7 +123,7 @@ static void not_implemented(void)
 fprintf(stderr, "Not implemented.\n");
 }
 
-static bool blacklisted(unsigned op, int rmode)
+static bool is_allowed(unsigned op, int rmode)
 {
 /* odd has not been implemented for any 80-bit ops */
 if (rmode == softfloat_round_odd) {
@@ -161,10 +161,10 @@ static bool blacklisted(unsigned op, int rmode)
 case F32_TO_EXTF80:
 case F64_TO_EXTF80:
 case F128_TO_EXTF80:
-return true;
+return false;
 }
 }
-return false;
+return true;
 }
 
 static void do_testfloat(int op, int rmode, bool exact)
@@ -194,7 +194,7 @@ static void do_testfloat(int op, int rmode, bool exact)
 verCases_writeFunctionName(stderr);
 fputs("\n", stderr);
 
-if (blacklisted(op, rmode)) {
+if (!is_allowed(op, rmode)) {
 not_implemented();
 return;
 }
-- 
2.26.2

[PATCH v3 4/5] qemu-options: Replace the word 'blacklist'

2021-03-03 Thread Philippe Mathieu-Daudé 
Follow the inclusive terminology from the "Conscious Language in your
Open Source Projects" guidelines [*] and replace the word "blacklist"
appropriately.

[*] https://github.com/conscious-lang/conscious-lang-docs/blob/main/faq.md

Reviewed-by: Daniel P. Berrangé 
Signed-off-by: Philippe Mathieu-Daudé 
---
 qemu-options.hx | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/qemu-options.hx b/qemu-options.hx
index 252db9357ca..8462dc5f158 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -4299,12 +4299,12 @@ DEF("sandbox", HAS_ARG, QEMU_OPTION_sandbox, \
 "use 'obsolete' to allow obsolete system calls that are 
provided\n" \
 "by the kernel, but typically no longer used by 
modern\n" \
 "C library implementations.\n" \
-"use 'elevateprivileges' to allow or deny QEMU process to 
elevate\n" \
-"its privileges by blacklisting all set*uid|gid system 
calls.\n" \
+"use 'elevateprivileges' to allow or deny the QEMU process 
ability\n" \
+"to elevate privileges using set*uid|gid system 
calls.\n" \
 "The value 'children' will deny set*uid|gid system 
calls for\n" \
 "main QEMU process but will allow forks and execves to 
run unprivileged\n" \
 "use 'spawn' to avoid QEMU to spawn new threads or 
processes by\n" \
-" blacklisting *fork and execve\n" \
+" blocking *fork and execve\n" \
 "use 'resourcecontrol' to disable process affinity and 
schedular priority\n",
 QEMU_ARCH_ALL)
 SRST
-- 
2.26.2

Re: QEMU RBD is slow with QCOW2 images

2021-03-03 Thread Jason Dillaman 
On Wed, Mar 3, 2021 at 12:41 PM Stefano Garzarella  wrote:
>
> Hi Jason,
> as reported in this BZ [1], when qemu-img creates a QCOW2 image on RBD
> writing data is very slow compared to a raw file.
>
> Comparing raw vs QCOW2 image creation with RBD I found that we use a
> different object size, for the raw file I see '4 MiB objects', for QCOW2
> I see '64 KiB objects' as reported on comment 14 [2].
> This should be the main issue of slowness, indeed forcing in the code 4
> MiB object size also for QCOW2 increased the speed a lot.
>
> Looking better I discovered that for raw files, we call rbd_create()
> with obj_order = 0 (if 'cluster_size' options is not defined), so the
> default object size is used.
> Instead for QCOW2, we use obj_order = 16, since the default
> 'cluster_size' defined for QCOW2, is 64 KiB.
>
> Using '-o cluster_size=2M' with qemu-img changed only the qcow2 cluster
> size, since in qcow2_co_create_opts() we remove the 'cluster_size' from
> QemuOpts calling qemu_opts_to_qdict_filtered().
> For some reason that I have yet to understand, after this deletion,
> however remains in QemuOpts the default value of 'cluster_size' for
> qcow2 (64 KiB), that it's used in qemu_rbd_co_create_opts()
>
> At this point my doubts are:
> Does it make sense to use the same cluster_size as qcow2 as object_size
> in RBD?

No, not really. But it also doesn't really make any sense to put a
QCOW2 image within an RBD image. To clarify from the BZ, OpenStack
does not put QCOW2 images on RBD, it converts QCOW2 images into raw
images to store in RBD.

> If we want to keep the 2 options separated, how can it be done? Should
> we rename the option in block/rbd.c?

You can already pass overrides to the RBD block driver by just
appending them after the
"rbd:[:option1=value1[:option2=value2]]" portion, perhaps
that could be re-used.

> Thanks,
> Stefano
>
> [1] https://bugzilla.redhat.com/show_bug.cgi?id=1744525
> [2] https://bugzilla.redhat.com/show_bug.cgi?id=1744525#c14
>


-- 
Jason

[PATCH v3 2/5] scripts/tracetool: Replace the word 'whitelist'

2021-03-03 Thread Philippe Mathieu-Daudé 
Follow the inclusive terminology from the "Conscious Language in your
Open Source Projects" guidelines [*] and replace the words "whitelist"
appropriately.

[*] https://github.com/conscious-lang/conscious-lang-docs/blob/main/faq.md

Reviewed-by: Daniel P. Berrangé 
Reviewed-by: Stefan Hajnoczi 
Reviewed-by: Alex Bennée 
Reviewed-by: Thomas Huth 
Signed-off-by: Philippe Mathieu-Daudé 
---
 scripts/tracetool/__init__.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/scripts/tracetool/__init__.py b/scripts/tracetool/__init__.py
index 96b1cd69a52..5bc94d95cfc 100644
--- a/scripts/tracetool/__init__.py
+++ b/scripts/tracetool/__init__.py
@@ -100,7 +100,7 @@ def validate_type(name):
 if bit == "const":
 continue
 if bit not in ALLOWED_TYPES:
-raise ValueError("Argument type '%s' is not in whitelist. "
+raise ValueError("Argument type '%s' is not allowed. "
  "Only standard C types and fixed size integer "
  "types should be used. struct, union, and "
  "other complex pointer types should be "
-- 
2.26.2

[PATCH v3 1/5] ui: Replace the word 'whitelist'

2021-03-03 Thread Philippe Mathieu-Daudé 
Follow the inclusive terminology from the "Conscious Language in your
Open Source Projects" guidelines [*] and replace the words "whitelist"
appropriately.

[*] https://github.com/conscious-lang/conscious-lang-docs/blob/main/faq.md

Reviewed-by: Gerd Hoffmann 
Reviewed-by: Alex Bennée 
Reviewed-by: Thomas Huth 
Signed-off-by: Philippe Mathieu-Daudé 
---
 ui/console.c   | 2 +-
 ui/vnc-auth-sasl.c | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/ui/console.c b/ui/console.c
index c5d11bc7017..5a8311ced20 100644
--- a/ui/console.c
+++ b/ui/console.c
@@ -1708,7 +1708,7 @@ bool dpy_gfx_check_format(QemuConsole *con,
 return false;
 }
 } else {
-/* default is to whitelist native 32 bpp only */
+/* default is to allow native 32 bpp only */
 if (format != qemu_default_pixman_format(32, true)) {
 return false;
 }
diff --git a/ui/vnc-auth-sasl.c b/ui/vnc-auth-sasl.c
index f67111a3662..df7dc08e9fc 100644
--- a/ui/vnc-auth-sasl.c
+++ b/ui/vnc-auth-sasl.c
@@ -288,7 +288,7 @@ static int protocol_client_auth_sasl_step(VncState *vs, 
uint8_t *data, size_t le
 goto authreject;
 }
 
-/* Check username whitelist ACL */
+/* Check the username access control list */
 if (vnc_auth_sasl_check_access(vs) < 0) {
 goto authreject;
 }
@@ -409,7 +409,7 @@ static int protocol_client_auth_sasl_start(VncState *vs, 
uint8_t *data, size_t l
 goto authreject;
 }
 
-/* Check username whitelist ACL */
+/* Check the username access control list */
 if (vnc_auth_sasl_check_access(vs) < 0) {
 goto authreject;
 }
-- 
2.26.2

QEMU RBD is slow with QCOW2 images

2021-03-03 Thread Stefano Garzarella 

Hi Jason,
as reported in this BZ [1], when qemu-img creates a QCOW2 image on RBD 
writing data is very slow compared to a raw file.


Comparing raw vs QCOW2 image creation with RBD I found that we use a 
different object size, for the raw file I see '4 MiB objects', for QCOW2 
I see '64 KiB objects' as reported on comment 14 [2].
This should be the main issue of slowness, indeed forcing in the code 4 
MiB object size also for QCOW2 increased the speed a lot.


Looking better I discovered that for raw files, we call rbd_create() 
with obj_order = 0 (if 'cluster_size' options is not defined), so the 
default object size is used.
Instead for QCOW2, we use obj_order = 16, since the default 
'cluster_size' defined for QCOW2, is 64 KiB.


Using '-o cluster_size=2M' with qemu-img changed only the qcow2 cluster 
size, since in qcow2_co_create_opts() we remove the 'cluster_size' from 
QemuOpts calling qemu_opts_to_qdict_filtered().
For some reason that I have yet to understand, after this deletion, 
however remains in QemuOpts the default value of 'cluster_size' for 
qcow2 (64 KiB), that it's used in qemu_rbd_co_create_opts()


At this point my doubts are:
Does it make sense to use the same cluster_size as qcow2 as object_size 
in RBD?
If we want to keep the 2 options separated, how can it be done? Should 
we rename the option in block/rbd.c?


Thanks,
Stefano

[1] https://bugzilla.redhat.com/show_bug.cgi?id=1744525
[2] https://bugzilla.redhat.com/show_bug.cgi?id=1744525#c14

Re: [PATCH v2 5/8] seccomp: Replace the word 'blacklist'

2021-03-03 Thread Thomas Huth 

On 05/02/2021 18.18, Philippe Mathieu-Daudé wrote:

Follow the inclusive terminology from the "Conscious Language in your
Open Source Projects" guidelines [*] and replace the word "blacklist"
appropriately.

[*] https://github.com/conscious-lang/conscious-lang-docs/blob/main/faq.md

Reviewed-by: Daniel P. Berrangé 
Acked-by: Eduardo Otubo 
Signed-off-by: Philippe Mathieu-Daudé 
---
  softmmu/qemu-seccomp.c | 16 
  1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/softmmu/qemu-seccomp.c b/softmmu/qemu-seccomp.c
index 377ef6937ca..4c684bc9e71 100644
--- a/softmmu/qemu-seccomp.c
+++ b/softmmu/qemu-seccomp.c
@@ -45,8 +45,8 @@ const struct scmp_arg_cmp sched_setscheduler_arg[] = {
  { .arg = 1, .op = SCMP_CMP_NE, .datum_a = SCHED_IDLE }
  };
  
-static const struct QemuSeccompSyscall blacklist[] = {

-/* default set of syscalls to blacklist */
+static const struct QemuSeccompSyscall denylist[] = {
+/* default set of syscalls to denylist */


Since it's used as a verb in the comment, I'd rather say something like this 
here:


/* default set of syscalls that should get blocked */

... "denylist" still does not sound like a verb to me.

 Thomas

Re: [PATCH v2 3/8] scripts/tracetool: Replace the word 'whitelist'

2021-03-03 Thread Thomas Huth 

On 05/02/2021 18.18, Philippe Mathieu-Daudé wrote:

Follow the inclusive terminology from the "Conscious Language in your
Open Source Projects" guidelines [*] and replace the words "whitelist"
appropriately.

[*] https://github.com/conscious-lang/conscious-lang-docs/blob/main/faq.md

Reviewed-by: Daniel P. Berrangé 
Reviewed-by: Stefan Hajnoczi 
Signed-off-by: Philippe Mathieu-Daudé 
---
  scripts/tracetool/__init__.py | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/scripts/tracetool/__init__.py b/scripts/tracetool/__init__.py
index 96b1cd69a52..5bc94d95cfc 100644
--- a/scripts/tracetool/__init__.py
+++ b/scripts/tracetool/__init__.py
@@ -100,7 +100,7 @@ def validate_type(name):
  if bit == "const":
  continue
  if bit not in ALLOWED_TYPES:
-raise ValueError("Argument type '%s' is not in whitelist. "
+raise ValueError("Argument type '%s' is not allowed. "
   "Only standard C types and fixed size integer "
   "types should be used. struct, union, and "
   "other complex pointer types should be "



Reviewed-by: Thomas Huth

Re: [PATCH v2 1/8] ui: Replace the word 'whitelist'

2021-03-03 Thread Thomas Huth 

On 05/02/2021 18.18, Philippe Mathieu-Daudé wrote:

Follow the inclusive terminology from the "Conscious Language in your
Open Source Projects" guidelines [*] and replace the words "whitelist"
appropriately.

[*] https://github.com/conscious-lang/conscious-lang-docs/blob/main/faq.md

Reviewed-by: Gerd Hoffmann 
Signed-off-by: Philippe Mathieu-Daudé 
---
v2: Do not use acronyms (danpb)
---
  ui/console.c   | 2 +-
  ui/vnc-auth-sasl.c | 4 ++--
  2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/ui/console.c b/ui/console.c
index c5d11bc7017..5a8311ced20 100644
--- a/ui/console.c
+++ b/ui/console.c
@@ -1708,7 +1708,7 @@ bool dpy_gfx_check_format(QemuConsole *con,
  return false;
  }
  } else {
-/* default is to whitelist native 32 bpp only */
+/* default is to allow native 32 bpp only */
  if (format != qemu_default_pixman_format(32, true)) {
  return false;
  }
diff --git a/ui/vnc-auth-sasl.c b/ui/vnc-auth-sasl.c
index f67111a3662..df7dc08e9fc 100644
--- a/ui/vnc-auth-sasl.c
+++ b/ui/vnc-auth-sasl.c
@@ -288,7 +288,7 @@ static int protocol_client_auth_sasl_step(VncState *vs, 
uint8_t *data, size_t le
  goto authreject;
  }
  
-/* Check username whitelist ACL */

+/* Check the username access control list */
  if (vnc_auth_sasl_check_access(vs) < 0) {
  goto authreject;
  }
@@ -409,7 +409,7 @@ static int protocol_client_auth_sasl_start(VncState *vs, 
uint8_t *data, size_t l
  goto authreject;
  }
  
-/* Check username whitelist ACL */

+/* Check the username access control list */
  if (vnc_auth_sasl_check_access(vs) < 0) {
  goto authreject;
  }



Reviewed-by: Thomas Huth

Re: [PATCH 0/2] Clarify error messages pertaining to 'node-name'

2021-03-03 Thread Connor Kuehl 

On 3/3/21 3:53 AM, Kevin Wolf wrote:

Am 02.03.2021 um 00:36 hat Connor Kuehl geschrieben:

Some error messages contain ambiguous representations of the 'node-name'
parameter. This can be particularly confusing when exchanging QMP
messages (C = client, S = server):

C: {"execute": "block_resize", "arguments": { "device": "my_file", "size": 
26843545600 }}
S: {"error": {"class": "GenericError", "desc": "Cannot find device=my_file nor 
node_name="}}

^


Arguably, this error message isn't great anyway because of the empty
string node name. We didn't even look for a node name, so why mention it
in the error message?

But your patches are certainly a good improvement already. I would have
merged them, but git grep 'nor node_name=' shows that you missed to
update a few tests, so they fail after the series. I suppose you only
caught the ones that are run by default in 'make check' and missed the
ones that require running the qemu-iotests 'check' script manually.


Ah, good catch! Yes, I was only using `make check`, I'll use the `check` 
script to uncover the other failures and fix them accordingly.



[..]


This is a good explanation for the change you're making. I think it
deserves being committed to the repository in the commit message for
patch 1.


I'll move this to patch #1 in the next revision of this series.

Thank you!

Connor

Re: [PATCH v3 00/12] block/export: vhost-user-blk server tests and input validation

2021-03-03 Thread Kevin Wolf 
Am 23.02.2021 um 15:46 hat Stefan Hajnoczi geschrieben:
> v3:
>  * Rebased onto qemu.git/master (7ef8134565dc)
>  * I asked Coiby Xu for clarification on a comment about vhost-user-blk-test 
> in
>v2. Please wait for that discussion to finish before merging.
> 
> v2:
>  * Add abrt handler that terminates qemu-storage-daemon to
>vhost-user-blk-test. No more orphaned processes on test failure. [Peter]
>  * Fix sector number calculation in vhost-user-blk-server.c
>  * Introduce VIRTIO_BLK_SECTOR_BITS/SIZE to make code clearer [Max]
>  * Fix vhost-user-blk-server.c blk_size double byteswap
>  * Fix vhost-user-blk blkcfg->num_queues endianness [Peter]
>  * Squashed cleanups into Coiby vhost-user-blk-test commit so the code is
>easier to review
> 
> The vhost-user-blk server test was already in Michael Tsirkin's recent vhost
> pull request, but was dropped because it exposed vhost-user regressions
> (b7c1bd9d7848 and the Based-on tag below). Now that the vhost-user regressions
> are fixed we can re-introduce the test case.
> 
> This series adds missing input validation that led to a Coverity report. The
> virtio-blk read, write, discard, and write zeroes commands need to check
> sector/byte ranges and other inputs. This solves the issue Peter Maydell 
> raised
> in "[PATCH for-5.2] block/export/vhost-user-blk-server.c: Avoid potential
> integer overflow".
> 
> Merging just the input validation patches would be possible too, but I prefer
> to merge the corresponding tests so the code is exercised by the CI.

Thanks, applied to the block branch.

Kevin

[RESEND PATCH v3 5/5] hw/sd: sdhci: Reset the data pointer of s->fifo_buffer[] when a different block size is programmed

2021-03-03 Thread Bin Meng 
If the block size is programmed to a different value from the
previous one, reset the data pointer of s->fifo_buffer[] so that
s->fifo_buffer[] can be filled in using the new block size in
the next transfer.

With this fix, the following reproducer:

outl 0xcf8 0x80001010
outl 0xcfc 0xe000
outl 0xcf8 0x80001001
outl 0xcfc 0x0600
write 0xe02c 0x1 0x05
write 0xe005 0x1 0x02
write 0xe007 0x1 0x01
write 0xe028 0x1 0x10
write 0x0 0x1 0x23
write 0x2 0x1 0x08
write 0xe00c 0x1 0x01
write 0xe00e 0x1 0x20
write 0xe00f 0x1 0x00
write 0xe00c 0x1 0x32
write 0xe004 0x2 0x0200
write 0xe028 0x1 0x00
write 0xe003 0x1 0x40

cannot be reproduced with the following QEMU command line:

$ qemu-system-x86_64 -nographic -machine accel=qtest -m 512M \
  -nodefaults -device sdhci-pci,sd-spec-version=3 \
  -drive if=sd,index=0,file=null-co://,format=raw,id=mydrive \
  -device sd-card,drive=mydrive -qtest stdio

Cc: qemu-sta...@nongnu.org
Fixes: CVE-2020-17380
Fixes: CVE-2020-25085
Fixes: CVE-2021-3409
Fixes: d7dfca0807a0 ("hw/sdhci: introduce standard SD host controller")
Reported-by: Alexander Bulekov 
Reported-by: Cornelius Aschermann (Ruhr-University Bochum)
Reported-by: Muhammad Ramdhan
Reported-by: Sergej Schumilo (Ruhr-University Bochum)
Reported-by: Simon Wrner (Ruhr-University Bochum)
Buglink: https://bugs.launchpad.net/qemu/+bug/1892960
Buglink: https://bugs.launchpad.net/qemu/+bug/1909418
Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1928146
Signed-off-by: Bin Meng 
Tested-by: Alexander Bulekov 

---

(no changes since v2)

Changes in v2:
- new patch: sdhci: Reset the data pointer of s->fifo_buffer[] when a different 
block size is programmed

 hw/sd/sdhci.c | 12 
 1 file changed, 12 insertions(+)

diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index d0c8e293c0..5b8678110b 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -1140,6 +1140,8 @@ sdhci_write(void *opaque, hwaddr offset, uint64_t val, 
unsigned size)
 break;
 case SDHC_BLKSIZE:
 if (!TRANSFERRING_DATA(s->prnsts)) {
+uint16_t blksize = s->blksize;
+
 MASKED_WRITE(s->blksize, mask, extract32(value, 0, 12));
 MASKED_WRITE(s->blkcnt, mask >> 16, value >> 16);
 
@@ -1151,6 +1153,16 @@ sdhci_write(void *opaque, hwaddr offset, uint64_t val, 
unsigned size)
 
 s->blksize = deposit32(s->blksize, 0, 12, s->buf_maxsz);
 }
+
+/*
+ * If the block size is programmed to a different value from
+ * the previous one, reset the data pointer of s->fifo_buffer[]
+ * so that s->fifo_buffer[] can be filled in using the new block
+ * size in the next transfer.
+ */
+if (blksize != s->blksize) {
+s->data_count = 0;
+}
 }
 
 break;
-- 
2.25.1

[RESEND PATCH v3 4/5] hw/sd: sdhci: Limit block size only when SDHC_BLKSIZE register is writable

2021-03-03 Thread Bin Meng 
The codes to limit the maximum block size is only necessary when
SDHC_BLKSIZE register is writable.

Signed-off-by: Bin Meng 
Tested-by: Alexander Bulekov 
Reviewed-by: Philippe Mathieu-Daudé 

---

(no changes since v2)

Changes in v2:
- new patch: sdhci: Limit block size only when SDHC_BLKSIZE register is writable

 hw/sd/sdhci.c | 14 +++---
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index 7a2003b28b..d0c8e293c0 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -1142,15 +1142,15 @@ sdhci_write(void *opaque, hwaddr offset, uint64_t val, 
unsigned size)
 if (!TRANSFERRING_DATA(s->prnsts)) {
 MASKED_WRITE(s->blksize, mask, extract32(value, 0, 12));
 MASKED_WRITE(s->blkcnt, mask >> 16, value >> 16);
-}
 
-/* Limit block size to the maximum buffer size */
-if (extract32(s->blksize, 0, 12) > s->buf_maxsz) {
-qemu_log_mask(LOG_GUEST_ERROR, "%s: Size 0x%x is larger than "
-  "the maximum buffer 0x%x\n", __func__, s->blksize,
-  s->buf_maxsz);
+/* Limit block size to the maximum buffer size */
+if (extract32(s->blksize, 0, 12) > s->buf_maxsz) {
+qemu_log_mask(LOG_GUEST_ERROR, "%s: Size 0x%x is larger than "
+  "the maximum buffer 0x%x\n", __func__, 
s->blksize,
+  s->buf_maxsz);
 
-s->blksize = deposit32(s->blksize, 0, 12, s->buf_maxsz);
+s->blksize = deposit32(s->blksize, 0, 12, s->buf_maxsz);
+}
 }
 
 break;
-- 
2.25.1

[RESEND PATCH v3 3/5] hw/sd: sdhci: Correctly set the controller status for ADMA

2021-03-03 Thread Bin Meng 
When an ADMA transfer is started, the codes forget to set the
controller status to indicate a transfer is in progress.

With this fix, the following 2 reproducers:

https://paste.debian.net/plain/1185136
https://paste.debian.net/plain/1185141

cannot be reproduced with the following QEMU command line:

$ qemu-system-x86_64 -nographic -machine accel=qtest -m 512M \
  -nodefaults -device sdhci-pci,sd-spec-version=3 \
  -drive if=sd,index=0,file=null-co://,format=raw,id=mydrive \
  -device sd-card,drive=mydrive -qtest stdio

Cc: qemu-sta...@nongnu.org
Fixes: CVE-2020-17380
Fixes: CVE-2020-25085
Fixes: CVE-2021-3409
Fixes: d7dfca0807a0 ("hw/sdhci: introduce standard SD host controller")
Reported-by: Alexander Bulekov 
Reported-by: Cornelius Aschermann (Ruhr-University Bochum)
Reported-by: Muhammad Ramdhan
Reported-by: Sergej Schumilo (Ruhr-University Bochum)
Reported-by: Simon Wrner (Ruhr-University Bochum)
Buglink: https://bugs.launchpad.net/qemu/+bug/1892960
Buglink: https://bugs.launchpad.net/qemu/+bug/1909418
Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1928146
Signed-off-by: Bin Meng 
Tested-by: Alexander Bulekov 
Reviewed-by: Philippe Mathieu-Daudé 
---

(no changes since v1)

 hw/sd/sdhci.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index 3feb6c3a1f..7a2003b28b 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -768,7 +768,9 @@ static void sdhci_do_adma(SDHCIState *s)
 
 switch (dscr.attr & SDHC_ADMA_ATTR_ACT_MASK) {
 case SDHC_ADMA_ATTR_ACT_TRAN:  /* data transfer */
+s->prnsts |= SDHC_DATA_INHIBIT | SDHC_DAT_LINE_ACTIVE;
 if (s->trnmod & SDHC_TRNS_READ) {
+s->prnsts |= SDHC_DOING_READ;
 while (length) {
 if (s->data_count == 0) {
 sdbus_read_data(>sdbus, s->fifo_buffer, block_size);
@@ -796,6 +798,7 @@ static void sdhci_do_adma(SDHCIState *s)
 }
 }
 } else {
+s->prnsts |= SDHC_DOING_WRITE;
 while (length) {
 begin = s->data_count;
 if ((length + begin) < block_size) {
-- 
2.25.1

[RESEND PATCH v3 2/5] hw/sd: sdhci: Don't write to SDHC_SYSAD register when transfer is in progress

2021-03-03 Thread Bin Meng 
Per "SD Host Controller Standard Specification Version 7.00"
chapter 2.2.1 SDMA System Address Register:

This register can be accessed only if no transaction is executing
(i.e., after a transaction has stopped).

With this fix, the following reproducer:

outl 0xcf8 0x80001010
outl 0xcfc 0xfbefff00
outl 0xcf8 0x80001001
outl 0xcfc 0x0600
write 0xfbefff2c 0x1 0x05
write 0xfbefff0f 0x1 0x37
write 0xfbefff0a 0x1 0x01
write 0xfbefff0f 0x1 0x29
write 0xfbefff0f 0x1 0x02
write 0xfbefff0f 0x1 0x03
write 0xfbefff04 0x1 0x01
write 0xfbefff05 0x1 0x01
write 0xfbefff07 0x1 0x02
write 0xfbefff0c 0x1 0x33
write 0xfbefff0e 0x1 0x20
write 0xfbefff0f 0x1 0x00
write 0xfbefff2a 0x1 0x01
write 0xfbefff0c 0x1 0x00
write 0xfbefff03 0x1 0x00
write 0xfbefff05 0x1 0x00
write 0xfbefff2a 0x1 0x02
write 0xfbefff0c 0x1 0x32
write 0xfbefff01 0x1 0x01
write 0xfbefff02 0x1 0x01
write 0xfbefff03 0x1 0x01

cannot be reproduced with the following QEMU command line:

$ qemu-system-x86_64 -nographic -machine accel=qtest -m 512M \
   -nodefaults -device sdhci-pci,sd-spec-version=3 \
   -drive if=sd,index=0,file=null-co://,format=raw,id=mydrive \
   -device sd-card,drive=mydrive -qtest stdio

Cc: qemu-sta...@nongnu.org
Fixes: CVE-2020-17380
Fixes: CVE-2020-25085
Fixes: CVE-2021-3409
Fixes: d7dfca0807a0 ("hw/sdhci: introduce standard SD host controller")
Reported-by: Alexander Bulekov 
Reported-by: Cornelius Aschermann (Ruhr-University Bochum)
Reported-by: Muhammad Ramdhan
Reported-by: Sergej Schumilo (Ruhr-University Bochum)
Reported-by: Simon Wrner (Ruhr-University Bochum)
Buglink: https://bugs.launchpad.net/qemu/+bug/1892960
Buglink: https://bugs.launchpad.net/qemu/+bug/1909418
Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1928146
Signed-off-by: Bin Meng 
Tested-by: Alexander Bulekov 

---

Changes in v3:
- Embed the reproducer in the commit message

 hw/sd/sdhci.c | 20 +++-
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index f72d76c178..3feb6c3a1f 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -1121,15 +1121,17 @@ sdhci_write(void *opaque, hwaddr offset, uint64_t val, 
unsigned size)
 
 switch (offset & ~0x3) {
 case SDHC_SYSAD:
-s->sdmasysad = (s->sdmasysad & mask) | value;
-MASKED_WRITE(s->sdmasysad, mask, value);
-/* Writing to last byte of sdmasysad might trigger transfer */
-if (!(mask & 0xFF00) && TRANSFERRING_DATA(s->prnsts) && s->blkcnt 
&&
-s->blksize && SDHC_DMA_TYPE(s->hostctl1) == SDHC_CTRL_SDMA) {
-if (s->trnmod & SDHC_TRNS_MULTI) {
-sdhci_sdma_transfer_multi_blocks(s);
-} else {
-sdhci_sdma_transfer_single_block(s);
+if (!TRANSFERRING_DATA(s->prnsts)) {
+s->sdmasysad = (s->sdmasysad & mask) | value;
+MASKED_WRITE(s->sdmasysad, mask, value);
+/* Writing to last byte of sdmasysad might trigger transfer */
+if (!(mask & 0xFF00) && s->blkcnt && s->blksize &&
+SDHC_DMA_TYPE(s->hostctl1) == SDHC_CTRL_SDMA) {
+if (s->trnmod & SDHC_TRNS_MULTI) {
+sdhci_sdma_transfer_multi_blocks(s);
+} else {
+sdhci_sdma_transfer_single_block(s);
+}
 }
 }
 break;
-- 
2.25.1

[RESEND PATCH v3 1/5] hw/sd: sdhci: Don't transfer any data when command time out

2021-03-03 Thread Bin Meng 
At the end of sdhci_send_command(), it starts a data transfer if the
command register indicates data is associated. But the data transfer
should only be initiated when the command execution has succeeded.

With this fix, the following reproducer:

outl 0xcf8 0x80001810
outl 0xcfc 0xe1068000
outl 0xcf8 0x80001804
outw 0xcfc 0x7
write 0xe106802c 0x1 0x0f
write 0xe1068004 0xc 0x2801d10101fbff28a384
write 0xe106800c 0x1f 
0x9dacbbcad9e8f7061524334251606f7e8d9cabbac9d8e7f60514233241505f
write 0xe1068003 0x28 
0x80d000251480d000252280d000253080d000253e80d000254c80d000255a80d000256880d0002576
write 0xe1068003 0x1 0xfe

cannot be reproduced with the following QEMU command line:

$ qemu-system-x86_64 -nographic -M pc-q35-5.0 \
  -device sdhci-pci,sd-spec-version=3 \
  -drive if=sd,index=0,file=null-co://,format=raw,id=mydrive \
  -device sd-card,drive=mydrive \
  -monitor none -serial none -qtest stdio

Cc: qemu-sta...@nongnu.org
Fixes: CVE-2020-17380
Fixes: CVE-2020-25085
Fixes: CVE-2021-3409
Fixes: d7dfca0807a0 ("hw/sdhci: introduce standard SD host controller")
Reported-by: Alexander Bulekov 
Reported-by: Cornelius Aschermann (Ruhr-University Bochum)
Reported-by: Muhammad Ramdhan
Reported-by: Sergej Schumilo (Ruhr-University Bochum)
Reported-by: Simon Wrner (Ruhr-University Bochum)
Buglink: https://bugs.launchpad.net/qemu/+bug/1892960
Buglink: https://bugs.launchpad.net/qemu/+bug/1909418
Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1928146
Signed-off-by: Bin Meng 
Acked-by: Alistair Francis 
Tested-by: Alexander Bulekov 
Tested-by: Philippe Mathieu-Daudé 
---

(no changes since v1)

 hw/sd/sdhci.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index 9acf4467a3..f72d76c178 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -326,6 +326,7 @@ static void sdhci_send_command(SDHCIState *s)
 SDRequest request;
 uint8_t response[16];
 int rlen;
+bool timeout = false;
 
 s->errintsts = 0;
 s->acmd12errsts = 0;
@@ -349,6 +350,7 @@ static void sdhci_send_command(SDHCIState *s)
 trace_sdhci_response16(s->rspreg[3], s->rspreg[2],
s->rspreg[1], s->rspreg[0]);
 } else {
+timeout = true;
 trace_sdhci_error("timeout waiting for command response");
 if (s->errintstsen & SDHC_EISEN_CMDTIMEOUT) {
 s->errintsts |= SDHC_EIS_CMDTIMEOUT;
@@ -369,7 +371,7 @@ static void sdhci_send_command(SDHCIState *s)
 
 sdhci_update_irq(s);
 
-if (s->blksize && (s->cmdreg & SDHC_CMD_DATA_PRESENT)) {
+if (!timeout && s->blksize && (s->cmdreg & SDHC_CMD_DATA_PRESENT)) {
 s->data_count = 0;
 sdhci_data_transfer(s);
 }
-- 
2.25.1

[RESEND PATCH v3 0/5] hw/sd: sdhci: Fixes to CVE-2020-17380, CVE-2020-25085, CVE-2021-3409

2021-03-03 Thread Bin Meng 
This series includes several fixes to CVE-2020-17380, CVE-2020-25085
and CVE-2021-3409 that are heap-based buffer overflow issues existing
in the sdhci model.

These CVEs are pretty much similar, and were filed using different
reproducers. With this series, current known reproducers I have
cannot be reproduced any more.

The implementation of this model still has some issues, e.g.: some codes
do not seem to strictly match the spec, but since this series only aimes
to address CVEs, such issue should be fixed in a separate series in the
future, if I have time :)

Changes in v3 RESEND:
- Drop the duplicated SoB email address

Changes in v3:
- Embed the reproducer in the commit message

Changes in v2:
- new patch: sdhci: Limit block size only when SDHC_BLKSIZE register is writable
- new patch: sdhci: Reset the data pointer of s->fifo_buffer[] when a different 
block size is programmed

Bin Meng (5):
  hw/sd: sdhci: Don't transfer any data when command time out
  hw/sd: sdhci: Don't write to SDHC_SYSAD register when transfer is in
progress
  hw/sd: sdhci: Correctly set the controller status for ADMA
  hw/sd: sdhci: Limit block size only when SDHC_BLKSIZE register is
writable
  hw/sd: sdhci: Reset the data pointer of s->fifo_buffer[] when a
different block size is programmed

 hw/sd/sdhci.c | 53 ++-
 1 file changed, 36 insertions(+), 17 deletions(-)

-- 
2.25.1

Re: [PATCH v2 5/8] seccomp: Replace the word 'blacklist'

2021-03-03 Thread Alex Bennée 


Philippe Mathieu-Daudé  writes:

> Follow the inclusive terminology from the "Conscious Language in your
> Open Source Projects" guidelines [*] and replace the word "blacklist"
> appropriately.
>
> [*] https://github.com/conscious-lang/conscious-lang-docs/blob/main/faq.md
>
> Reviewed-by: Daniel P. Berrangé 
> Acked-by: Eduardo Otubo 
> Signed-off-by: Philippe Mathieu-Daudé 

Reviewed-by: Alex Bennée 

-- 
Alex Bennée

Re: [PATCH v2 4/8] scripts/device-crash-test: Replace the word 'whitelist'

2021-03-03 Thread Alex Bennée 


Philippe Mathieu-Daudé  writes:

> Follow the inclusive terminology from the "Conscious Language in your
> Open Source Projects" guidelines [*] and replace the word "whitelist"
> appropriately.
>
> [*] https://github.com/conscious-lang/conscious-lang-docs/blob/main/faq.md
>
> Reviewed-by: Daniel P. Berrangé 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  scripts/device-crash-test | 30 +++---
>  1 file changed, 15 insertions(+), 15 deletions(-)
>
> diff --git a/scripts/device-crash-test b/scripts/device-crash-test
> index 04118669ba7..6812de42f8c 100755
> --- a/scripts/device-crash-test
> +++ b/scripts/device-crash-test
> @@ -41,18 +41,18 @@ logger = logging.getLogger('device-crash-test')
>  dbg = logger.debug
>  
>  
> -# Purposes of the following whitelist:
> +# Purposes of the following allowlist:
>  # * Avoiding verbose log messages when we find known non-fatal
>  #   (exitcode=1) errors
>  # * Avoiding fatal errors when we find known crashes
>  # * Skipping machines/devices that are known not to work out of
>  #   the box, when running in --quick mode
>  #
> -# Keeping the whitelist updated is desirable, but not required,
> +# Keeping the allowlist updated is desirable, but not required,
>  # because unexpected cases where QEMU exits with exitcode=1 will
>  # just trigger a INFO message.
>  
> -# Valid whitelist entry keys:
> +# Valid allowlist entry keys:
>  # * accel: regexp, full match only
>  # * machine: regexp, full match only
>  # * device: regexp, full match only
> @@ -62,7 +62,7 @@ dbg = logger.debug
>  # * expected: if True, QEMU is expected to always fail every time
>  #   when testing the corresponding test case
>  # * loglevel: log level of log output when there's a match.
> -ERROR_WHITELIST = [
> +ERROR_ALLOWLIST = [
>  # Machines that won't work out of the box:
>  # MACHINE | ERROR MESSAGE
>  {'machine':'niagara', 'expected':True},   # Unable to load a 
> firmware for -M niagara
> @@ -187,9 +187,9 @@ ERROR_WHITELIST = [
>  
>  
>  def whitelistTestCaseMatch(wl, t):
> -"""Check if a test case specification can match a whitelist entry
> +"""Check if a test case specification can match a allowlist entry

Seems like the function names (and params?) could be updated as well.

>  
> -This only checks if a whitelist entry is a candidate match
> +This only checks if a allowlist entry is a candidate match
>  for a given test case, it won't check if the test case
>  results/output match the entry.  See whitelistResultMatch().
>  """
> @@ -206,16 +206,16 @@ def whitelistTestCaseMatch(wl, t):
>  
>  def whitelistCandidates(t):
>  """Generate the list of candidates that can match a test case"""
> -for i, wl in enumerate(ERROR_WHITELIST):
> +for i, wl in enumerate(ERROR_ALLOWLIST):
>  if whitelistTestCaseMatch(wl, t):
>  yield (i, wl)
>  
>  
>  def findExpectedResult(t):
> -"""Check if there's an expected=True whitelist entry for a test case
> +"""Check if there's an expected=True allowlist entry for a test case
>  
>  Returns (i, wl) tuple, where i is the index in
> -ERROR_WHITELIST and wl is the whitelist entry itself.
> +ERROR_ALLOWLIST and wl is the allowlist entry itself.
>  """
>  for i, wl in whitelistCandidates(t):
>  if wl.get('expected'):
> @@ -223,7 +223,7 @@ def findExpectedResult(t):
>  
>  
>  def whitelistResultMatch(wl, r):
> -"""Check if test case results/output match a whitelist entry
> +"""Check if test case results/output match a allowlist entry
>  
>  It is valid to call this function only if
>  whitelistTestCaseMatch() is True for the entry (e.g. on
> @@ -237,10 +237,10 @@ def whitelistResultMatch(wl, r):
>  
>  
>  def checkResultWhitelist(r):
> -"""Look up whitelist entry for a given test case result
> +"""Look up allowlist entry for a given test case result
>  
>  Returns (i, wl) tuple, where i is the index in
> -ERROR_WHITELIST and wl is the whitelist entry itself.
> +ERROR_ALLOWLIST and wl is the allowlist entry itself.
>  """
>  for i, wl in whitelistCandidates(r['testcase']):
>  if whitelistResultMatch(wl, r):
> @@ -544,7 +544,7 @@ def main():
>  
>  if f:
>  i, wl = checkResultWhitelist(f)
> -dbg("testcase: %r, whitelist match: %r", t, wl)
> +dbg("testcase: %r, allowlist match: %r", t, wl)
>  wl_stats.setdefault(i, []).append(f)
>  level = wl.get('loglevel', logging.DEBUG)
>  logFailure(f, level)
> @@ -561,9 +561,9 @@ def main():
>  
>  if args.debug:
>  stats = sorted([(len(wl_stats.get(i, [])), wl) for i, wl in
> - enumerate(ERROR_WHITELIST)], key=lambda x: x[0])
> + enumerate(ERROR_ALLOWLIST)], key=lambda x: x[0])
>  for count, wl in stats:
> -dbg("whitelist entry stats: %d: %r",

Re: [PATCH v2 3/8] scripts/tracetool: Replace the word 'whitelist'

2021-03-03 Thread Alex Bennée 


Philippe Mathieu-Daudé  writes:

> Follow the inclusive terminology from the "Conscious Language in your
> Open Source Projects" guidelines [*] and replace the words "whitelist"
> appropriately.
>
> [*] https://github.com/conscious-lang/conscious-lang-docs/blob/main/faq.md
>
> Reviewed-by: Daniel P. Berrangé 
> Reviewed-by: Stefan Hajnoczi 
> Signed-off-by: Philippe Mathieu-Daudé 

Reviewed-by: Alex Bennée 

-- 
Alex Bennée

Re: [PATCH v2 1/8] ui: Replace the word 'whitelist'

2021-03-03 Thread Alex Bennée 


Philippe Mathieu-Daudé  writes:

> Follow the inclusive terminology from the "Conscious Language in your
> Open Source Projects" guidelines [*] and replace the words "whitelist"
> appropriately.
>
> [*] https://github.com/conscious-lang/conscious-lang-docs/blob/main/faq.md
>
> Reviewed-by: Gerd Hoffmann 
> Signed-off-by: Philippe Mathieu-Daudé 

Reviewed-by: Alex Bennée 

-- 
Alex Bennée

[PATCH v3 4/5] hw/sd: sdhci: Limit block size only when SDHC_BLKSIZE register is writable

2021-03-03 Thread Bin Meng 
The codes to limit the maximum block size is only necessary when
SDHC_BLKSIZE register is writable.

Signed-off-by: Bin Meng 
Tested-by: Alexander Bulekov 
Reviewed-by: Philippe Mathieu-Daudé 

Signed-off-by: Bin Meng 
---

(no changes since v2)

Changes in v2:
- new patch: sdhci: Limit block size only when SDHC_BLKSIZE register is writable

 hw/sd/sdhci.c | 14 +++---
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index 7a2003b..d0c8e29 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -1142,15 +1142,15 @@ sdhci_write(void *opaque, hwaddr offset, uint64_t val, 
unsigned size)
 if (!TRANSFERRING_DATA(s->prnsts)) {
 MASKED_WRITE(s->blksize, mask, extract32(value, 0, 12));
 MASKED_WRITE(s->blkcnt, mask >> 16, value >> 16);
-}
 
-/* Limit block size to the maximum buffer size */
-if (extract32(s->blksize, 0, 12) > s->buf_maxsz) {
-qemu_log_mask(LOG_GUEST_ERROR, "%s: Size 0x%x is larger than "
-  "the maximum buffer 0x%x\n", __func__, s->blksize,
-  s->buf_maxsz);
+/* Limit block size to the maximum buffer size */
+if (extract32(s->blksize, 0, 12) > s->buf_maxsz) {
+qemu_log_mask(LOG_GUEST_ERROR, "%s: Size 0x%x is larger than "
+  "the maximum buffer 0x%x\n", __func__, 
s->blksize,
+  s->buf_maxsz);
 
-s->blksize = deposit32(s->blksize, 0, 12, s->buf_maxsz);
+s->blksize = deposit32(s->blksize, 0, 12, s->buf_maxsz);
+}
 }
 
 break;
-- 
2.7.4

[PATCH v3 2/5] hw/sd: sdhci: Don't write to SDHC_SYSAD register when transfer is in progress

2021-03-03 Thread Bin Meng 
Per "SD Host Controller Standard Specification Version 7.00"
chapter 2.2.1 SDMA System Address Register:

This register can be accessed only if no transaction is executing
(i.e., after a transaction has stopped).

With this fix, the following reproducer:

outl 0xcf8 0x80001010
outl 0xcfc 0xfbefff00
outl 0xcf8 0x80001001
outl 0xcfc 0x0600
write 0xfbefff2c 0x1 0x05
write 0xfbefff0f 0x1 0x37
write 0xfbefff0a 0x1 0x01
write 0xfbefff0f 0x1 0x29
write 0xfbefff0f 0x1 0x02
write 0xfbefff0f 0x1 0x03
write 0xfbefff04 0x1 0x01
write 0xfbefff05 0x1 0x01
write 0xfbefff07 0x1 0x02
write 0xfbefff0c 0x1 0x33
write 0xfbefff0e 0x1 0x20
write 0xfbefff0f 0x1 0x00
write 0xfbefff2a 0x1 0x01
write 0xfbefff0c 0x1 0x00
write 0xfbefff03 0x1 0x00
write 0xfbefff05 0x1 0x00
write 0xfbefff2a 0x1 0x02
write 0xfbefff0c 0x1 0x32
write 0xfbefff01 0x1 0x01
write 0xfbefff02 0x1 0x01
write 0xfbefff03 0x1 0x01

cannot be reproduced with the following QEMU command line:

$ qemu-system-x86_64 -nographic -machine accel=qtest -m 512M \
   -nodefaults -device sdhci-pci,sd-spec-version=3 \
   -drive if=sd,index=0,file=null-co://,format=raw,id=mydrive \
   -device sd-card,drive=mydrive -qtest stdio

Cc: qemu-sta...@nongnu.org
Fixes: CVE-2020-17380
Fixes: CVE-2020-25085
Fixes: CVE-2021-3409
Fixes: d7dfca0807a0 ("hw/sdhci: introduce standard SD host controller")
Reported-by: Alexander Bulekov 
Reported-by: Cornelius Aschermann (Ruhr-University Bochum)
Reported-by: Muhammad Ramdhan
Reported-by: Sergej Schumilo (Ruhr-University Bochum)
Reported-by: Simon Wrner (Ruhr-University Bochum)
Buglink: https://bugs.launchpad.net/qemu/+bug/1892960
Buglink: https://bugs.launchpad.net/qemu/+bug/1909418
Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1928146
Signed-off-by: Bin Meng 
Tested-by: Alexander Bulekov 

Signed-off-by: Bin Meng 
---

Changes in v3:
- Embed the reproducer in the commit message

 hw/sd/sdhci.c | 20 +++-
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index f72d76c..3feb6c3 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -1121,15 +1121,17 @@ sdhci_write(void *opaque, hwaddr offset, uint64_t val, 
unsigned size)
 
 switch (offset & ~0x3) {
 case SDHC_SYSAD:
-s->sdmasysad = (s->sdmasysad & mask) | value;
-MASKED_WRITE(s->sdmasysad, mask, value);
-/* Writing to last byte of sdmasysad might trigger transfer */
-if (!(mask & 0xFF00) && TRANSFERRING_DATA(s->prnsts) && s->blkcnt 
&&
-s->blksize && SDHC_DMA_TYPE(s->hostctl1) == SDHC_CTRL_SDMA) {
-if (s->trnmod & SDHC_TRNS_MULTI) {
-sdhci_sdma_transfer_multi_blocks(s);
-} else {
-sdhci_sdma_transfer_single_block(s);
+if (!TRANSFERRING_DATA(s->prnsts)) {
+s->sdmasysad = (s->sdmasysad & mask) | value;
+MASKED_WRITE(s->sdmasysad, mask, value);
+/* Writing to last byte of sdmasysad might trigger transfer */
+if (!(mask & 0xFF00) && s->blkcnt && s->blksize &&
+SDHC_DMA_TYPE(s->hostctl1) == SDHC_CTRL_SDMA) {
+if (s->trnmod & SDHC_TRNS_MULTI) {
+sdhci_sdma_transfer_multi_blocks(s);
+} else {
+sdhci_sdma_transfer_single_block(s);
+}
 }
 }
 break;
-- 
2.7.4

[PATCH v3 0/5] hw/sd: sdhci: Fixes to CVE-2020-17380, CVE-2020-25085, CVE-2021-3409

2021-03-03 Thread Bin Meng 
This series includes several fixes to CVE-2020-17380, CVE-2020-25085
and CVE-2021-3409 that are heap-based buffer overflow issues existing
in the sdhci model.

These CVEs are pretty much similar, and were filed using different
reproducers. With this series, current known reproducers I have
cannot be reproduced any more.

The implementation of this model still has some issues, e.g.: some codes
do not seem to strictly match the spec, but since this series only aimes
to address CVEs, such issue should be fixed in a separate series in the
future, if I have time :)

Changes in v3:
- Embed the reproducer in the commit message

Changes in v2:
- new patch: sdhci: Limit block size only when SDHC_BLKSIZE register is writable
- new patch: sdhci: Reset the data pointer of s->fifo_buffer[] when a different 
block size is programmed

Bin Meng (5):
  hw/sd: sdhci: Don't transfer any data when command time out
  hw/sd: sdhci: Don't write to SDHC_SYSAD register when transfer is in
progress
  hw/sd: sdhci: Correctly set the controller status for ADMA
  hw/sd: sdhci: Limit block size only when SDHC_BLKSIZE register is
writable
  hw/sd: sdhci: Reset the data pointer of s->fifo_buffer[] when a
different block size is programmed

 hw/sd/sdhci.c | 53 -
 1 file changed, 36 insertions(+), 17 deletions(-)

-- 
2.7.4

[PATCH v3 3/5] hw/sd: sdhci: Correctly set the controller status for ADMA

2021-03-03 Thread Bin Meng 
When an ADMA transfer is started, the codes forget to set the
controller status to indicate a transfer is in progress.

With this fix, the following 2 reproducers:

https://paste.debian.net/plain/1185136
https://paste.debian.net/plain/1185141

cannot be reproduced with the following QEMU command line:

$ qemu-system-x86_64 -nographic -machine accel=qtest -m 512M \
  -nodefaults -device sdhci-pci,sd-spec-version=3 \
  -drive if=sd,index=0,file=null-co://,format=raw,id=mydrive \
  -device sd-card,drive=mydrive -qtest stdio

Cc: qemu-sta...@nongnu.org
Fixes: CVE-2020-17380
Fixes: CVE-2020-25085
Fixes: CVE-2021-3409
Fixes: d7dfca0807a0 ("hw/sdhci: introduce standard SD host controller")
Reported-by: Alexander Bulekov 
Reported-by: Cornelius Aschermann (Ruhr-University Bochum)
Reported-by: Muhammad Ramdhan
Reported-by: Sergej Schumilo (Ruhr-University Bochum)
Reported-by: Simon Wrner (Ruhr-University Bochum)
Buglink: https://bugs.launchpad.net/qemu/+bug/1892960
Buglink: https://bugs.launchpad.net/qemu/+bug/1909418
Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1928146
Signed-off-by: Bin Meng 
Tested-by: Alexander Bulekov 
Reviewed-by: Philippe Mathieu-Daudé 
Signed-off-by: Bin Meng 
---

(no changes since v1)

 hw/sd/sdhci.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index 3feb6c3..7a2003b 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -768,7 +768,9 @@ static void sdhci_do_adma(SDHCIState *s)
 
 switch (dscr.attr & SDHC_ADMA_ATTR_ACT_MASK) {
 case SDHC_ADMA_ATTR_ACT_TRAN:  /* data transfer */
+s->prnsts |= SDHC_DATA_INHIBIT | SDHC_DAT_LINE_ACTIVE;
 if (s->trnmod & SDHC_TRNS_READ) {
+s->prnsts |= SDHC_DOING_READ;
 while (length) {
 if (s->data_count == 0) {
 sdbus_read_data(>sdbus, s->fifo_buffer, block_size);
@@ -796,6 +798,7 @@ static void sdhci_do_adma(SDHCIState *s)
 }
 }
 } else {
+s->prnsts |= SDHC_DOING_WRITE;
 while (length) {
 begin = s->data_count;
 if ((length + begin) < block_size) {
-- 
2.7.4

[PATCH v3 5/5] hw/sd: sdhci: Reset the data pointer of s->fifo_buffer[] when a different block size is programmed

2021-03-03 Thread Bin Meng 
If the block size is programmed to a different value from the
previous one, reset the data pointer of s->fifo_buffer[] so that
s->fifo_buffer[] can be filled in using the new block size in
the next transfer.

With this fix, the following reproducer:

outl 0xcf8 0x80001010
outl 0xcfc 0xe000
outl 0xcf8 0x80001001
outl 0xcfc 0x0600
write 0xe02c 0x1 0x05
write 0xe005 0x1 0x02
write 0xe007 0x1 0x01
write 0xe028 0x1 0x10
write 0x0 0x1 0x23
write 0x2 0x1 0x08
write 0xe00c 0x1 0x01
write 0xe00e 0x1 0x20
write 0xe00f 0x1 0x00
write 0xe00c 0x1 0x32
write 0xe004 0x2 0x0200
write 0xe028 0x1 0x00
write 0xe003 0x1 0x40

cannot be reproduced with the following QEMU command line:

$ qemu-system-x86_64 -nographic -machine accel=qtest -m 512M \
  -nodefaults -device sdhci-pci,sd-spec-version=3 \
  -drive if=sd,index=0,file=null-co://,format=raw,id=mydrive \
  -device sd-card,drive=mydrive -qtest stdio

Cc: qemu-sta...@nongnu.org
Fixes: CVE-2020-17380
Fixes: CVE-2020-25085
Fixes: CVE-2021-3409
Fixes: d7dfca0807a0 ("hw/sdhci: introduce standard SD host controller")
Reported-by: Alexander Bulekov 
Reported-by: Cornelius Aschermann (Ruhr-University Bochum)
Reported-by: Muhammad Ramdhan
Reported-by: Sergej Schumilo (Ruhr-University Bochum)
Reported-by: Simon Wrner (Ruhr-University Bochum)
Buglink: https://bugs.launchpad.net/qemu/+bug/1892960
Buglink: https://bugs.launchpad.net/qemu/+bug/1909418
Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1928146
Signed-off-by: Bin Meng 
Tested-by: Alexander Bulekov 

Signed-off-by: Bin Meng 
---

(no changes since v2)

Changes in v2:
- new patch: sdhci: Reset the data pointer of s->fifo_buffer[] when a different 
block size is programmed

 hw/sd/sdhci.c | 12 
 1 file changed, 12 insertions(+)

diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index d0c8e29..5b86781 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -1140,6 +1140,8 @@ sdhci_write(void *opaque, hwaddr offset, uint64_t val, 
unsigned size)
 break;
 case SDHC_BLKSIZE:
 if (!TRANSFERRING_DATA(s->prnsts)) {
+uint16_t blksize = s->blksize;
+
 MASKED_WRITE(s->blksize, mask, extract32(value, 0, 12));
 MASKED_WRITE(s->blkcnt, mask >> 16, value >> 16);
 
@@ -1151,6 +1153,16 @@ sdhci_write(void *opaque, hwaddr offset, uint64_t val, 
unsigned size)
 
 s->blksize = deposit32(s->blksize, 0, 12, s->buf_maxsz);
 }
+
+/*
+ * If the block size is programmed to a different value from
+ * the previous one, reset the data pointer of s->fifo_buffer[]
+ * so that s->fifo_buffer[] can be filled in using the new block
+ * size in the next transfer.
+ */
+if (blksize != s->blksize) {
+s->data_count = 0;
+}
 }
 
 break;
-- 
2.7.4

[PATCH v3 1/5] hw/sd: sdhci: Don't transfer any data when command time out

2021-03-03 Thread Bin Meng 
At the end of sdhci_send_command(), it starts a data transfer if the
command register indicates data is associated. But the data transfer
should only be initiated when the command execution has succeeded.

With this fix, the following reproducer:

outl 0xcf8 0x80001810
outl 0xcfc 0xe1068000
outl 0xcf8 0x80001804
outw 0xcfc 0x7
write 0xe106802c 0x1 0x0f
write 0xe1068004 0xc 0x2801d10101fbff28a384
write 0xe106800c 0x1f 
0x9dacbbcad9e8f7061524334251606f7e8d9cabbac9d8e7f60514233241505f
write 0xe1068003 0x28 
0x80d000251480d000252280d000253080d000253e80d000254c80d000255a80d000256880d0002576
write 0xe1068003 0x1 0xfe

cannot be reproduced with the following QEMU command line:

$ qemu-system-x86_64 -nographic -M pc-q35-5.0 \
  -device sdhci-pci,sd-spec-version=3 \
  -drive if=sd,index=0,file=null-co://,format=raw,id=mydrive \
  -device sd-card,drive=mydrive \
  -monitor none -serial none -qtest stdio

Cc: qemu-sta...@nongnu.org
Fixes: CVE-2020-17380
Fixes: CVE-2020-25085
Fixes: CVE-2021-3409
Fixes: d7dfca0807a0 ("hw/sdhci: introduce standard SD host controller")
Reported-by: Alexander Bulekov 
Reported-by: Cornelius Aschermann (Ruhr-University Bochum)
Reported-by: Muhammad Ramdhan
Reported-by: Sergej Schumilo (Ruhr-University Bochum)
Reported-by: Simon Wrner (Ruhr-University Bochum)
Buglink: https://bugs.launchpad.net/qemu/+bug/1892960
Buglink: https://bugs.launchpad.net/qemu/+bug/1909418
Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1928146
Signed-off-by: Bin Meng 
Acked-by: Alistair Francis 
Tested-by: Alexander Bulekov 
Tested-by: Philippe Mathieu-Daudé 
Signed-off-by: Bin Meng 
---

(no changes since v1)

 hw/sd/sdhci.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index 9acf446..f72d76c 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -326,6 +326,7 @@ static void sdhci_send_command(SDHCIState *s)
 SDRequest request;
 uint8_t response[16];
 int rlen;
+bool timeout = false;
 
 s->errintsts = 0;
 s->acmd12errsts = 0;
@@ -349,6 +350,7 @@ static void sdhci_send_command(SDHCIState *s)
 trace_sdhci_response16(s->rspreg[3], s->rspreg[2],
s->rspreg[1], s->rspreg[0]);
 } else {
+timeout = true;
 trace_sdhci_error("timeout waiting for command response");
 if (s->errintstsen & SDHC_EISEN_CMDTIMEOUT) {
 s->errintsts |= SDHC_EIS_CMDTIMEOUT;
@@ -369,7 +371,7 @@ static void sdhci_send_command(SDHCIState *s)
 
 sdhci_update_irq(s);
 
-if (s->blksize && (s->cmdreg & SDHC_CMD_DATA_PRESENT)) {
+if (!timeout && s->blksize && (s->cmdreg & SDHC_CMD_DATA_PRESENT)) {
 s->data_count = 0;
 sdhci_data_transfer(s);
 }
-- 
2.7.4

Re: [PATCH v2 0/8] misc: Replace the words 'blacklist/whitelist'

2021-03-03 Thread Philippe Mathieu-Daudé 
Hi Laurent,

On 2/5/21 6:18 PM, Philippe Mathieu-Daudé wrote:
> Follow the inclusive terminology from the "Conscious Language in your
> Open Source Projects" guidelines [*] and replace the words "blacklist"
> and "whitelist" appropriately.
> 

> Series expected to go via the qemu-trivial@ tree.

I forgot to Cc qemu-trivial@ :/ The series is now fully reviewed :)

> [*] https://github.com/conscious-lang/conscious-lang-docs/blob/main/faq.md
> 
> Philippe Mathieu-Daudé (8):
>   ui: Replace the word 'whitelist'
>   tools/virtiofsd: Replace the word 'whitelist'
>   scripts/tracetool: Replace the word 'whitelist'
>   scripts/device-crash-test: Replace the word 'whitelist'
>   seccomp: Replace the word 'blacklist'
>   qemu-options: Replace the word 'blacklist'
>   tests/fp/fp-test: Replace the word 'blacklist'
>   hw/vfio/pci-quirks: Replace the word 'blacklist'

Re: [PATCH v2 6/8] qemu-options: Replace the word 'blacklist'

2021-03-03 Thread Daniel P . Berrangé 
On Fri, Feb 05, 2021 at 06:18:15PM +0100, Philippe Mathieu-Daudé wrote:
> Follow the inclusive terminology from the "Conscious Language in your
> Open Source Projects" guidelines [*] and replace the word "blacklist"
> appropriately.
> 
> [*] https://github.com/conscious-lang/conscious-lang-docs/blob/main/faq.md
> 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
> v2: Reword (danpb)
> ---
>  qemu-options.hx | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/qemu-options.hx b/qemu-options.hx
> index c09c4646e28..5f86cd2fbbf 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -4274,12 +4274,12 @@ DEF("sandbox", HAS_ARG, QEMU_OPTION_sandbox, \
>  "use 'obsolete' to allow obsolete system calls that are 
> provided\n" \
>  "by the kernel, but typically no longer used by 
> modern\n" \
>  "C library implementations.\n" \
> -"use 'elevateprivileges' to allow or deny QEMU process 
> to elevate\n" \
> -"its privileges by blacklisting all set*uid|gid 
> system calls.\n" \
> +"use 'elevateprivileges' to allow or deny the QEMU 
> process ability\n" \
> +"to elevate privileges using set*uid|gid system 
> calls.\n" \
>  "The value 'children' will deny set*uid|gid system 
> calls for\n" \
>  "main QEMU process but will allow forks and execves 
> to run unprivileged\n" \
>  "use 'spawn' to avoid QEMU to spawn new threads or 
> processes by\n" \
> -" blacklisting *fork and execve\n" \
> +" blocking *fork and execve\n" \
>  "use 'resourcecontrol' to disable process affinity and 
> schedular priority\n",
>  QEMU_ARCH_ALL)
>  SRST

Reviewed-by: Daniel P. Berrangé 


Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

Re: [PATCH 0/2] Clarify error messages pertaining to 'node-name'

2021-03-03 Thread Kevin Wolf 
Am 02.03.2021 um 00:36 hat Connor Kuehl geschrieben:
> Some error messages contain ambiguous representations of the 'node-name'
> parameter. This can be particularly confusing when exchanging QMP
> messages (C = client, S = server):
> 
> C: {"execute": "block_resize", "arguments": { "device": "my_file", "size": 
> 26843545600 }}
> S: {"error": {"class": "GenericError", "desc": "Cannot find device=my_file 
> nor node_name="}}
>   
>  ^

Arguably, this error message isn't great anyway because of the empty
string node name. We didn't even look for a node name, so why mention it
in the error message?

But your patches are certainly a good improvement already. I would have
merged them, but git grep 'nor node_name=' shows that you missed to
update a few tests, so they fail after the series. I suppose you only
caught the ones that are run by default in 'make check' and missed the
ones that require running the qemu-iotests 'check' script manually.

> This error message suggests one could send a message with a key called
> 'node_name':
> 
> C: {"execute": "block_resize", "arguments": { "node_name": "my_file", "size": 
> 26843545600 }}
>^
> 
> but using the underscore is actually incorrect, the parameter should be
> 'node-name':
> 
> S: {"error": {"class": "GenericError", "desc": "Parameter 'node_name' is 
> unexpected"}}
> 
> This behavior was uncovered in bz1651437[1], but I ended up going down a
> rabbit hole looking for other areas where this miscommunication might
> occur and changing those accordingly as well.
> 
> [1] https://bugzilla.redhat.com/1651437

This is a good explanation for the change you're making. I think it
deserves being committed to the repository in the commit message for
patch 1.

Kevin

Re: [PATCH v3 0/2] docs: show how to spawn qemu-storage-daemon with fd passing

2021-03-03 Thread Kevin Wolf 
Am 01.03.2021 um 18:27 hat Stefan Hajnoczi geschrieben:
> v3:
>  * Explain how to detect launch errors and that the listen socket must be
>closed in the parent process in order for this to work [Daniel]
> 
> v2:
>  * Use /var/run/qmp.sock instead of /tmp/qmp-$PID.sock to prevent security
>issues with world-writeable directories [Rich, Daniel]
>  * Add Patch 2 to fix insecure examples in the documentation [Rich, Daniel]
> 
> Add an example of how to spawn qemu-storage-daemon with fd passing. This
> approach eliminates the need to busy wait for the QMP, NBD, or vhost-user
> socket to become available.

Thanks, applied to the block branch.

Kevin

Re: [PATCH v3] qemu-storage-daemon: add --pidfile option

2021-03-03 Thread Kevin Wolf 
Am 02.03.2021 um 15:27 hat Stefan Hajnoczi geschrieben:
> Daemons often have a --pidfile option where the pid is written to a file
> so that scripts can stop the daemon by sending a signal.
> 
> The pid file also acts as a lock to prevent multiple instances of the
> daemon from launching for a given pid file.
> 
> QEMU, qemu-nbd, qemu-ga, virtiofsd, and qemu-pr-helper all support the
> --pidfile option. Add it to qemu-storage-daemon too.
> 
> Reported-by: Richard W.M. Jones 
> Signed-off-by: Stefan Hajnoczi 

Thanks, applied to the block branch.

Kevin

Re: [PATCH v3 12/21] sd: emmc: add CMD21 tuning sequence

2021-03-03 Thread Dr. David Alan Gilbert 
* Sai Pavan Boddu (saip...@xilinx.com) wrote:
> Hi David,
> 
> > -Original Message-
> > From: Dr. David Alan Gilbert 
> > Sent: Monday, March 1, 2021 4:12 PM
> > To: Sai Pavan Boddu 
> > Cc: Markus Armbruster ; Kevin Wolf
> > ; Max Reitz ; Vladimir Sementsov-
> > Ogievskiy ; Eric Blake ;
> > Joel Stanley ; Cédric Le Goater ; Vincent
> > Palatin ; Thomas Huth ; Stefan
> > Hajnoczi ; Peter Maydell ;
> > Alistair Francis ; Edgar Iglesias 
> > ;
> > Luc Michel ; Paolo Bonzini
> > ; qemu-block@nongnu.org; qemu-de...@nongnu.org;
> > Sai Pavan Boddu 
> > Subject: Re: [PATCH v3 12/21] sd: emmc: add CMD21 tuning sequence
> > 
> > * Sai Pavan Boddu (sai.pavan.bo...@xilinx.com) wrote:
> > > eMMC cards support tuning sequence for entering HS200 mode.
> > >
> > > Signed-off-by: Sai Pavan Boddu 
> > > Signed-off-by: Edgar E. Iglesias 
> > > ---
> > >  hw/sd/sd.c | 47 +++
> > >  1 file changed, 47 insertions(+)
> > >
> > > diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> > > index bf963ec..174c28e 100644
> > > --- a/hw/sd/sd.c
> > > +++ b/hw/sd/sd.c
> > > @@ -1386,6 +1386,14 @@ static sd_rsp_type_t
> > sd_normal_command(SDState *sd, SDRequest req)
> > >  }
> > >  break;
> > >
> > > +case 21:/* CMD21: mmc SEND TUNING_BLOCK */
> > > +if (sd->emmc && (sd->state == sd_transfer_state)) {
> > > +sd->state = sd_sendingdata_state;
> > > +sd->data_offset = 0;
> > > +return sd_r1;
> > > +}
> > > +break;
> > > +
> > >  case 23:/* CMD23: SET_BLOCK_COUNT */
> > >  if (sd->spec_version < SD_PHY_SPECv3_01_VERS) {
> > >  break;
> > > @@ -2120,6 +2128,30 @@ static const uint8_t
> > sd_tuning_block_pattern[SD_TUNING_BLOCK_SIZE] = {
> > >  0xbb, 0xff, 0xf7, 0xff, 0xf7, 0x7f, 0x7b, 0xde,
> > >  };
> > >
> > > +#define EXCSD_BUS_WIDTH_OFFSET 183
> > > +#define BUS_WIDTH_8_MASK0x4
> > > +#define BUS_WIDTH_4_MASK0x2
> > > +#define MMC_TUNING_BLOCK_SIZE   128
> > > +
> > > +static const uint8_t mmc_tunning_block_pattern[128] = {
> > > +   0xff, 0xff, 0x00, 0xff, 0xff, 0xff, 0x00, 0x00,
> > > +   0xff, 0xff, 0xcc, 0xcc, 0xcc, 0x33, 0xcc, 0xcc,
> > > +   0xcc, 0x33, 0x33, 0xcc, 0xcc, 0xcc, 0xff, 0xff,
> > > +   0xff, 0xee, 0xff, 0xff, 0xff, 0xee, 0xee, 0xff,
> > > +   0xff, 0xff, 0xdd, 0xff, 0xff, 0xff, 0xdd, 0xdd,
> > > +   0xff, 0xff, 0xff, 0xbb, 0xff, 0xff, 0xff, 0xbb,
> > > +   0xbb, 0xff, 0xff, 0xff, 0x77, 0xff, 0xff, 0xff,
> > > +   0x77, 0x77, 0xff, 0x77, 0xbb, 0xdd, 0xee, 0xff,
> > > +   0xff, 0xff, 0xff, 0x00, 0xff, 0xff, 0xff, 0x00,
> > > +   0x00, 0xff, 0xff, 0xcc, 0xcc, 0xcc, 0x33, 0xcc,
> > > +   0xcc, 0xcc, 0x33, 0x33, 0xcc, 0xcc, 0xcc, 0xff,
> > > +   0xff, 0xff, 0xee, 0xff, 0xff, 0xff, 0xee, 0xee,
> > > +   0xff, 0xff, 0xff, 0xdd, 0xff, 0xff, 0xff, 0xdd,
> > > +   0xdd, 0xff, 0xff, 0xff, 0xbb, 0xff, 0xff, 0xff,
> > > +   0xbb, 0xbb, 0xff, 0xff, 0xff, 0x77, 0xff, 0xff,
> > > +   0xff, 0x77, 0x77, 0xff, 0x77, 0xbb, 0xdd, 0xee,
> > 
> > Where does this magic pattern come from?  Is it part of some spec?
> [Sai Pavan Boddu] Yes its part of JEDEC eMMC spec. It's the tuning sequence 
> for HS200 mode.

OK, if you have to repost it, then please add a comment saying that.

DAve

> Regards,
> Sai Pavan
> > 
> > Dave
> > 
> > > +};
> > > +
> > >  uint8_t sd_read_byte(SDState *sd)
> > >  {
> > >  /* TODO: Append CRCs */
> > > @@ -2213,6 +2245,21 @@ uint8_t sd_read_byte(SDState *sd)
> > >  ret = sd_tuning_block_pattern[sd->data_offset++];
> > >  break;
> > >
> > > +case 21:/* CMD21: SEND_TUNNING_BLOCK (MMC) */
> > > +if (sd->data_offset >= MMC_TUNING_BLOCK_SIZE - 1) {
> > > +sd->state = sd_transfer_state;
> > > +}
> > > +if (sd->ext_csd[EXCSD_BUS_WIDTH_OFFSET] & BUS_WIDTH_8_MASK) {
> > > +ret = mmc_tunning_block_pattern[sd->data_offset++];
> > > +} else {
> > > +/* Return LSB Nibbles of two byte from the 8bit tuning block
> > > + * for 4bit mode
> > > + */
> > > +ret = mmc_tunning_block_pattern[sd->data_offset++] & 0x0F;
> > > +ret |= (mmc_tunning_block_pattern[sd->data_offset++] & 0x0F) 
> > > << 4;
> > > +}
> > > +break;
> > > +
> > >  case 22:/* ACMD22: SEND_NUM_WR_BLOCKS */
> > >  ret = sd->data[sd->data_offset ++];
> > >
> > > --
> > > 2.7.4
> > >
> > --
> > Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
> 
-- 
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK