linux-kernel - Re: [PATCH] ALSA: usb-audio: Prevent urb from writing out of bounds

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20251107005420.3537826-1-lizhi.xu@windriver.com>
Date: Fri, 7 Nov 2025 08:54:20 +0800
From: Lizhi Xu <lizhi.xu@...driver.com>
To: <tiwai@...e.de>
CC: <hdanton@...a.com>, <linux-kernel@...r.kernel.org>,
        <linux-sound@...r.kernel.org>, <linux-usb@...r.kernel.org>,
        <lizhi.xu@...driver.com>, <perex@...ex.cz>,
        <syzbot+bfd77469c8966de076f7@...kaller.appspotmail.com>,
        <syzkaller-bugs@...glegroups.com>
Subject: Re: [PATCH] ALSA: usb-audio: Prevent urb from writing out of bounds

On Thu, 06 Nov 2025 17:41:07 +0100, Takashi Iwai wrote:
> > > > > > The calculation rule for the actual data length written to the URB's
> > > > > > transfer buffer differs from that used to allocate the URB's transfer
> > > > > > buffer, and in this problem, the value used during allocation is smaller.
> > > > > >
> > > > > > This ultimately leads to write out-of-bounds errors when writing data to
> > > > > > the transfer buffer.
> > > > > >
> > > > > > To prevent out-of-bounds writes to the transfer buffer, a check between
> > > > > > the size of the bytes to be written and the size of the allocated bytes
> > > > > > should be added before performing the write operation.
> > > > > >
> > > > > > When the written bytes are too large, -EPIPE is returned instead of
> > > > > > -EAGAIN, because returning -EAGAIN might result in push back to ready
> > > > > > list again.
> > > > > >
> > > > > > Based on the context of calculating the bytes to be written here, both
> > > > > > copy_to_urb() and copy_to_urb_quirk() require a check for the size of
> > > > > > the bytes to be written before execution.
> > > > > >
> > > > > > syzbot reported:
> > > > > > BUG: KASAN: slab-out-of-bounds in copy_to_urb+0x261/0x460 sound/usb/pcm.c:1487
> > > > > > Write of size 264 at addr ffff88801107b400 by task syz.0.17/5461
> > > > > >
> > > > > > Call Trace:
> > > > > >  copy_to_urb+0x261/0x460 sound/usb/pcm.c:1487
> > > > > >  prepare_playback_urb+0x953/0x13d0 sound/usb/pcm.c:1611
> > > > > >
> > > > > > Reported-by: syzbot+bfd77469c8966de076f7@...kaller.appspotmail.com
> > > > > > Closes: https://syzkaller.appspot.com/bug?extid=bfd77469c8966de076f7
> > > > > > Tested-by: syzbot+bfd77469c8966de076f7@...kaller.appspotmail.com
> > > > > > Signed-off-by: Lizhi Xu <lizhi.xu@...driver.com>
> > > > >
> > > > > I'm afraid that this doesn't address the root cause at all.
> > > > > The description above sounds plausible, but not pointing to "why".
> > > > >
> > > > > The bytes is frames * stride, so the question is why a too large
> > > > > frames is calculated.  I couldn't have time to check the details, but
> > > > > there should be rather some weird condition / parameters to trigger
> > > > > this, and we should check that at first.
> > > > During debugging, I discovered that the value of ep->packsize[0] is 22,
> > > > which causes the counts calculated by
> > > > counts = snd_usb_endpoint_next_packet_size(ep, ctx, i, avail);
> > > > to be 22, resulting in a frames value of 22 * 6 = 132;
> > > > Meanwhile, the stride value is 2, which ultimately results in
> > > > bytes = frames * stride = 132 * 2 = 264;
> > > > @@ -1241,6 +1252,10 @@ static int data_ep_set_params(struct snd_usb_endpoint *ep)
> > > > 	u->buffer_size = maxsize * u->packets;
> > > > 	...
> > > > 	u->urb->transfer_buffer =
> > > >                 usb_alloc_coherent(chip->dev, u->buffer_size,
> > > >                                    GFP_KERNEL, &u->urb->transfer_dma);
> > > >
> > > > Here, when calculating u->buffer_size = maxsize * u->packets;
> > > > maxsize = 9, packets = 6, which results in only 54 bytes allocated to
> > > > transfer_buffer;
> > >
> > > Hm, so the problem is rather the calculation of the buffer size.
> > > The size sounds extremely small.  Which parameters (rates, formats,
> > > etc) are used for achieving this?
> > rates: 22050
> > format: 2
> > channels: 1
> > /////////////////////////////
> > stride: 2
> > packets: 6
> > data interval: 0
> > frame_bits: 16
> > >
> > > The calculation of u->buffer_size is a bit complex, as maxsize is
> > > adjusted in many different ways.  Is it limited due to wMaxPacketSize
> > > setup?
> > Yes, it's because the value of ep->maxpacksize is 9 that the maxsize
> > value is 9.
> 
> OK, then a fix like below would work?
> 
> 
> thanks,
> 
> Takashi
> 
> --- a/sound/usb/endpoint.c
> +++ b/sound/usb/endpoint.c
> @@ -1362,6 +1362,11 @@ int snd_usb_endpoint_set_params(struct snd_usb_audio *chip,
>  	ep->sample_rem = ep->cur_rate % ep->pps;
>  	ep->packsize[0] = ep->cur_rate / ep->pps;
>  	ep->packsize[1] = (ep->cur_rate + (ep->pps - 1)) / ep->pps;
> +	if (ep->packsize[1] > ep->maxpacksize) {
> +		usb_audio_dbg(chip, "Too small maxpacksize %u for rate %u / pps %u\n",
> +			      ep->maxpacksize, ep->cur_rate, ep->pps);
> +		return -EINVAL;
> +	}
> 
>  	/* calculate the frequency in 16.16 format */
>  	ep->freqm = ep->freqn;
Of course, this fix was added after packsize[0] was assigned a value,
and Hillf Danton has already tested it.

However, to be more precise, although both packsize[1] and packsize[0]
exceed maxpacksize, this example is about packsize[0], so judging packsize[0]
is more rigorous.

BR,
Lizhi