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

[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?

> > +    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);
> >      }