[Qemu-devel] [PATCH] audio/hda: fix guest triggerable assert

[Qemu-devel] [PATCH] audio/hda: fix guest triggerable assert

[Gerd Hoffmann]

Guest writes to a readonly register trigger the assert in
intel_hda_reg_write().  Add a check and just ignore them.

Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1628433
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/audio/intel-hda.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/hw/audio/intel-hda.c b/hw/audio/intel-hda.c
index 23a2cf6484..066532713c 100644
--- a/hw/audio/intel-hda.c
+++ b/hw/audio/intel-hda.c
@@ -929,6 +929,10 @@ static void intel_hda_reg_write(IntelHDAState *d, const IntelHDAReg *reg, uint32
     if (!reg) {
         return;
     }
+    if (!reg->wmask) {
+        /* read-only register */
+        return;
+    }
 
     if (d->debug) {
         time_t now = time(NULL);
-- 
2.9.3

Re: [Qemu-devel] [PATCH] audio/hda: fix guest triggerable assert

[Dr. David Alan Gilbert]

* Gerd Hoffmann (kraxel@redhat.com) wrote:
> Guest writes to a readonly register trigger the assert in
> intel_hda_reg_write().  Add a check and just ignore them.
> 
> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1628433
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>

Does make you wonder:
  a) Why the guest was writing to a read-only register
  b) Wth it only did it in the weird combination of
     devices tested.

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> ---
>  hw/audio/intel-hda.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/hw/audio/intel-hda.c b/hw/audio/intel-hda.c
> index 23a2cf6484..066532713c 100644
> --- a/hw/audio/intel-hda.c
> +++ b/hw/audio/intel-hda.c
> @@ -929,6 +929,10 @@ static void intel_hda_reg_write(IntelHDAState *d, const IntelHDAReg *reg, uint32
>      if (!reg) {
>          return;
>      }
> +    if (!reg->wmask) {
> +        /* read-only register */
> +        return;
> +    }
>  
>      if (d->debug) {
>          time_t now = time(NULL);
> -- 
> 2.9.3
> 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH] audio/hda: fix guest triggerable assert

[Philippe Mathieu-Daudé]

Hi Gerd,

On 22/11/18 14:32, Gerd Hoffmann wrote:
> Guest writes to a readonly register trigger the assert in
> intel_hda_reg_write().  Add a check and just ignore them.

Is this 3.1 material? It seems to apply.

> 
> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1628433
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  hw/audio/intel-hda.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/hw/audio/intel-hda.c b/hw/audio/intel-hda.c
> index 23a2cf6484..066532713c 100644
> --- a/hw/audio/intel-hda.c
> +++ b/hw/audio/intel-hda.c
> @@ -929,6 +929,10 @@ static void intel_hda_reg_write(IntelHDAState *d, const IntelHDAReg *reg, uint32
>      if (!reg) {
>          return;
>      }
> +    if (!reg->wmask) {
> +        /* read-only register */

Can you add:

           qemu_log_mask(LOG_GUEST_ERROR,
                         "intel-hda: Register %s is read-only\n",
                         reg->name);

Regardless:
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> +        return;
> +    }
>  
>      if (d->debug) {
>          time_t now = time(NULL);
> 

Re: [Qemu-devel] [PATCH] audio/hda: fix guest triggerable assert

[Gerd Hoffmann]

On Thu, Nov 22, 2018 at 03:52:12PM +0000, Dr. David Alan Gilbert wrote:
> * Gerd Hoffmann (kraxel@redhat.com) wrote:
> > Guest writes to a readonly register trigger the assert in
> > intel_hda_reg_write().  Add a check and just ignore them.
> > 
> > Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1628433
> > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> 
> Does make you wonder:
>   a) Why the guest was writing to a read-only register
>   b) Wth it only did it in the weird combination of
>      devices tested.

Note that we also have pci hotplug involved.  Probably a bug in the
guest, maybe due to a register access landing at the wrong device while
reshuffling the bars or something like that.

cheers,
  Gerd