diff mbox series

[PULL,v2,17/61] virtio-snd: check for invalid param shift operands

Message ID 9b6083465fb8311f2410615f8303a41f580a2a20.1721731723.git.mst@redhat.com
State New
Headers show
Series [PULL,v2,01/61] hw/virtio/virtio-crypto: Fix op_code assignment in virtio_crypto_create_asym_session | expand

Commit Message

Michael S. Tsirkin July 23, 2024, 10:56 a.m. UTC 
From: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>

When setting the parameters of a PCM stream, we compute the bit flag
with the format and rate values as shift operand to check if they are
set in supported_formats and supported_rates.

If the guest provides a format/rate value which when shifting 1 results
in a value bigger than the number of bits in
supported_formats/supported_rates, we must report an error.

Previously, this ended up triggering the not reached assertions later
when converting to internal QEMU values.

Reported-by: Zheyu Ma <zheyuma97@gmail.com>
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2416
Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
Message-Id: <virtio-snd-fuzz-2416-fix-v1-manos.pitsidianakis@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/audio/virtio-snd.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Comments

Volker Rümelin July 27, 2024, 6:55 a.m. UTC | #1 
> From: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
>
> When setting the parameters of a PCM stream, we compute the bit flag
> with the format and rate values as shift operand to check if they are
> set in supported_formats and supported_rates.
>
> If the guest provides a format/rate value which when shifting 1 results
> in a value bigger than the number of bits in
> supported_formats/supported_rates, we must report an error.
>
> Previously, this ended up triggering the not reached assertions later
> when converting to internal QEMU values.
>
> Reported-by: Zheyu Ma <zheyuma97@gmail.com>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2416
> Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
> Message-Id: <virtio-snd-fuzz-2416-fix-v1-manos.pitsidianakis@linaro.org>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  hw/audio/virtio-snd.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/hw/audio/virtio-snd.c b/hw/audio/virtio-snd.c
> index e6432ac959..e5196aa4bb 100644
> --- a/hw/audio/virtio-snd.c
> +++ b/hw/audio/virtio-snd.c
> @@ -282,11 +282,13 @@ uint32_t virtio_snd_set_pcm_params(VirtIOSound *s,
>          error_report("Number of channels is not supported.");
>          return cpu_to_le32(VIRTIO_SND_S_NOT_SUPP);
>      }
> -    if (!(supported_formats & BIT(params->format))) {

Hi Manos,

this patch doesn't work as intended. I guess you wanted to write

    if (params->format >= sizeof(supported_formats) * BITS_PER_BYTE ||
        !(supported_formats & BIT(params->format))) {

> +    if (BIT(params->format) > sizeof(supported_formats) ||
> +        !(supported_formats & BIT(params->format))) {
>          error_report("Stream format is not supported.");
>          return cpu_to_le32(VIRTIO_SND_S_NOT_SUPP);
>      }
> -    if (!(supported_rates & BIT(params->rate))) {

    if (params->rate >= sizeof(supported_rates) * BITS_PER_BYTE ||
        !(supported_rates & BIT(params->rate))) {

With best regards,
Volker

> +    if (BIT(params->rate) > sizeof(supported_rates) ||
> +        !(supported_rates & BIT(params->rate))) {
>          error_report("Stream rate is not supported.");
>          return cpu_to_le32(VIRTIO_SND_S_NOT_SUPP);
>      }
Michael S. Tsirkin Aug. 1, 2024, 8:22 a.m. UTC | #2 
On Sat, Jul 27, 2024 at 08:55:10AM +0200, Volker Rümelin wrote:
> > From: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
> >
> > When setting the parameters of a PCM stream, we compute the bit flag
> > with the format and rate values as shift operand to check if they are
> > set in supported_formats and supported_rates.
> >
> > If the guest provides a format/rate value which when shifting 1 results
> > in a value bigger than the number of bits in
> > supported_formats/supported_rates, we must report an error.
> >
> > Previously, this ended up triggering the not reached assertions later
> > when converting to internal QEMU values.
> >
> > Reported-by: Zheyu Ma <zheyuma97@gmail.com>
> > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2416
> > Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
> > Message-Id: <virtio-snd-fuzz-2416-fix-v1-manos.pitsidianakis@linaro.org>
> > Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> > Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> >  hw/audio/virtio-snd.c | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/hw/audio/virtio-snd.c b/hw/audio/virtio-snd.c
> > index e6432ac959..e5196aa4bb 100644
> > --- a/hw/audio/virtio-snd.c
> > +++ b/hw/audio/virtio-snd.c
> > @@ -282,11 +282,13 @@ uint32_t virtio_snd_set_pcm_params(VirtIOSound *s,
> >          error_report("Number of channels is not supported.");
> >          return cpu_to_le32(VIRTIO_SND_S_NOT_SUPP);
> >      }
> > -    if (!(supported_formats & BIT(params->format))) {
> 
> Hi Manos,
> 
> this patch doesn't work as intended. I guess you wanted to write
> 
>     if (params->format >= sizeof(supported_formats) * BITS_PER_BYTE ||
>         !(supported_formats & BIT(params->format))) {
> 
> > +    if (BIT(params->format) > sizeof(supported_formats) ||
> > +        !(supported_formats & BIT(params->format))) {
> >          error_report("Stream format is not supported.");
> >          return cpu_to_le32(VIRTIO_SND_S_NOT_SUPP);
> >      }
> > -    if (!(supported_rates & BIT(params->rate))) {
> 
>     if (params->rate >= sizeof(supported_rates) * BITS_PER_BYTE ||
>         !(supported_rates & BIT(params->rate))) {
> 
> With best regards,
> Volker


Any response here? Should I revert?

> > +    if (BIT(params->rate) > sizeof(supported_rates) ||
> > +        !(supported_rates & BIT(params->rate))) {
> >          error_report("Stream rate is not supported.");
> >          return cpu_to_le32(VIRTIO_SND_S_NOT_SUPP);
> >      }
Volker Rümelin Aug. 2, 2024, 5:03 a.m. UTC | #3 
Am 01.08.24 um 10:22 schrieb Michael S. Tsirkin:
> On Sat, Jul 27, 2024 at 08:55:10AM +0200, Volker Rümelin wrote:
>>> From: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
>>>
>>> When setting the parameters of a PCM stream, we compute the bit flag
>>> with the format and rate values as shift operand to check if they are
>>> set in supported_formats and supported_rates.
>>>
>>> If the guest provides a format/rate value which when shifting 1 results
>>> in a value bigger than the number of bits in
>>> supported_formats/supported_rates, we must report an error.
>>>
>>> Previously, this ended up triggering the not reached assertions later
>>> when converting to internal QEMU values.
>>>
>>> Reported-by: Zheyu Ma <zheyuma97@gmail.com>
>>> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2416
>>> Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
>>> Message-Id: <virtio-snd-fuzz-2416-fix-v1-manos.pitsidianakis@linaro.org>
>>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
>>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>>> ---
>>>  hw/audio/virtio-snd.c | 6 ++++--
>>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/hw/audio/virtio-snd.c b/hw/audio/virtio-snd.c
>>> index e6432ac959..e5196aa4bb 100644
>>> --- a/hw/audio/virtio-snd.c
>>> +++ b/hw/audio/virtio-snd.c
>>> @@ -282,11 +282,13 @@ uint32_t virtio_snd_set_pcm_params(VirtIOSound *s,
>>>          error_report("Number of channels is not supported.");
>>>          return cpu_to_le32(VIRTIO_SND_S_NOT_SUPP);
>>>      }
>>> -    if (!(supported_formats & BIT(params->format))) {
>> Hi Manos,
>>
>> this patch doesn't work as intended. I guess you wanted to write
>>
>>     if (params->format >= sizeof(supported_formats) * BITS_PER_BYTE ||
>>         !(supported_formats & BIT(params->format))) {
>>
>>> +    if (BIT(params->format) > sizeof(supported_formats) ||
>>> +        !(supported_formats & BIT(params->format))) {
>>>          error_report("Stream format is not supported.");
>>>          return cpu_to_le32(VIRTIO_SND_S_NOT_SUPP);
>>>      }
>>> -    if (!(supported_rates & BIT(params->rate))) {
>>     if (params->rate >= sizeof(supported_rates) * BITS_PER_BYTE ||
>>         !(supported_rates & BIT(params->rate))) {
>>
>> With best regards,
>> Volker
>
> Any response here? Should I revert?

No response so far. It's not necessary to revert. I'll send a patch.

With best regards,
Volker

>>> +    if (BIT(params->rate) > sizeof(supported_rates) ||
>>> +        !(supported_rates & BIT(params->rate))) {
>>>          error_report("Stream rate is not supported.");
>>>          return cpu_to_le32(VIRTIO_SND_S_NOT_SUPP);
>>>      }
Manos Pitsidianakis Aug. 2, 2024, 11:13 a.m. UTC | #4 
On Fri, 02 Aug 2024 08:03, Volker Rümelin <vr_qemu@t-online.de> wrote:
>Am 01.08.24 um 10:22 schrieb Michael S. Tsirkin:
>> On Sat, Jul 27, 2024 at 08:55:10AM +0200, Volker Rümelin wrote: >>>> 
>>> Hi Manos,
>>>
>>> this patch doesn't work as intended. I guess you wanted to write
>>>
>>>     if (params->format >= sizeof(supported_formats) * BITS_PER_BYTE ||
>>>         !(supported_formats & BIT(params->format))) {
>>>
>>>> +    if (BIT(params->format) > sizeof(supported_formats) ||
>>>> +        !(supported_formats & BIT(params->format))) {
>>>>          error_report("Stream format is not supported.");
>>>>          return cpu_to_le32(VIRTIO_SND_S_NOT_SUPP);
>>>>      }
>>>> -    if (!(supported_rates & BIT(params->rate))) {
>>>     if (params->rate >= sizeof(supported_rates) * BITS_PER_BYTE ||
>>>         !(supported_rates & BIT(params->rate))) {
>>>
>>> With best regards,
>>> Volker
>>
>> Any response here? Should I revert?
>
>No response so far. It's not necessary to revert. I'll send a patch.
>
>With best regards,
>Volker

Hello, I am on PTO. I reviewed Volker's patch, and it LGTM.

Thank you both,
Manos
diff mbox series

Patch

diff --git a/hw/audio/virtio-snd.c b/hw/audio/virtio-snd.c
index e6432ac959..e5196aa4bb 100644
--- a/hw/audio/virtio-snd.c
+++ b/hw/audio/virtio-snd.c
@@ -282,11 +282,13 @@  uint32_t virtio_snd_set_pcm_params(VirtIOSound *s,
         error_report("Number of channels is not supported.");
         return cpu_to_le32(VIRTIO_SND_S_NOT_SUPP);
     }
-    if (!(supported_formats & BIT(params->format))) {
+    if (BIT(params->format) > sizeof(supported_formats) ||
+        !(supported_formats & BIT(params->format))) {
         error_report("Stream format is not supported.");
         return cpu_to_le32(VIRTIO_SND_S_NOT_SUPP);
     }
-    if (!(supported_rates & BIT(params->rate))) {
+    if (BIT(params->rate) > sizeof(supported_rates) ||
+        !(supported_rates & BIT(params->rate))) {
         error_report("Stream rate is not supported.");
         return cpu_to_le32(VIRTIO_SND_S_NOT_SUPP);
     }