Re: [PATCH] sound: usb: increase snd_card alloc size

[Takashi Iwai]

On Fri, 22 Sep 2023 10:46:26 +0200,
Takashi Iwai wrote:
> 
> On Fri, 22 Sep 2023 02:51:53 +0200,
> Ricardo B. Marliere wrote:
> > 
> > Syzbot reports a slab-out-of-bounds read of a snd_card object. When
> > snd_usb_audio_create calls snd_card_new, it passes sizeof(*chip) as the
> > extra_size argument, which is not enough in this case.
> > 
> > Relevant logs below:
> > 
> > BUG: KASAN: slab-out-of-bounds in imon_probe+0x2983/0x3910
> > Read of size 1 at addr ffff8880436a2c71 by task kworker/1:2/777
> > (...)
> > The buggy address belongs to the object at ffff8880436a2000
> >  which belongs to the cache kmalloc-4k of size 4096
> > The buggy address is located 1 bytes to the right of
> >  allocated 3184-byte region [ffff8880436a2000, ffff8880436a2c70)
> > 
> > Reported-by: syzbot+59875ffef5cb9c9b29e9@xxxxxxxxxxxxxxxxxxxxxxxxx
> > Closes: https://lore.kernel.org/all/000000000000a838aa0603cc74d6@xxxxxxxxx/m
> > Signed-off-by: Ricardo B. Marliere <ricardo@xxxxxxxxxxxx>
> > ---
> >  sound/usb/card.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/sound/usb/card.c b/sound/usb/card.c
> > index 1b2edc0fd2e9..6578326d33e8 100644
> > --- a/sound/usb/card.c
> > +++ b/sound/usb/card.c
> > @@ -619,7 +619,7 @@ static int snd_usb_audio_create(struct usb_interface *intf,
> >  	}
> >  
> >  	err = snd_card_new(&intf->dev, index[idx], id[idx], THIS_MODULE,
> > -			   sizeof(*chip), &card);
> > +			   sizeof(*chip) + 2, &card);
> 
> Sorry, it's no-no.  We have to fix the cause of the OOB access instead
> of papering over with a random number of increase.
> 
> Unfortunately, most important piece of information is trimmed in the
> changelog, so I can't judge what's going on.  The only useful info
> there is that it's something to do with imon driver, but it's
> completely independent from USB-audio.  How does it access to the
> external memory allocated by snd-usb-audio driver at all?
> 
> Before jumping to the solution, we must understand the problem.

Now I took a look at the syzbot URL and got more info.

Through a quick glance, my wild guess is that two different drivers
are bound to two interfaces of the device, the first one to usb-audio
and the second one to imon.  And imon driver blindly assumes that the
first interface is bound with imon, too, and that can be the cause.
A patch like below (totally untested!) might fix the problem.

Can you reproduce the problem in your side?  Or did you pick this up
randomly without testing?

In anyway, let's put media people to Cc.


thanks,

Takashi

--- a/drivers/media/rc/imon.c
+++ b/drivers/media/rc/imon.c
@@ -2427,6 +2427,12 @@ static int imon_probe(struct usb_interface *interface,
 		goto fail;
 	}
 
+	if (first_if->dev.driver != interface->dev.driver) {
+		dev_err(&interface->dev, "inconsistent driver matching\n");
+		ret = -EINVAL;
+		goto fail;
+	}
+
 	if (ifnum == 0) {
 		ictx = imon_init_intf0(interface, id);
 		if (!ictx) {