linux-kernel - Re: [PATCH v2] usb: gadget: functioni: Fix a oob problem in rndis

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: <2025071150-removal-bullring-5649@gregkh>
Date: Fri, 11 Jul 2025 07:54:36 +0200
From: Greg KH <gregkh@...uxfoundation.org>
To: jackysliu <1972843537@...com>
Cc: linux-kernel@...r.kernel.org, linux-usb@...r.kernel.org,
	viro@...iv.linux.org.uk
Subject: Re: [PATCH v2] usb: gadget: functioni: Fix a oob problem in rndis

On Fri, Jul 11, 2025 at 11:46:35AM +0800, jackysliu wrote:
> On Thu, 10 Jul 2025 14:19:47 +0200, greg k-h wrote
> 
> >In digging in this further, I don't see how this actually changes
> >anything.  BufLength is used for nothing that I can determine, except in
> >some debugging code that is always compiled out (i.e. you can NOT enable
> >it unless you modify the kernel source.)
> >
> >So what exactly is this check checking?
> >
> >I can see that we really should be checking if the buffer is too small,
> >but that's not what you are doing here at all.
> >
> >And all this buffer is used for is to read a 32bit value out of, so
> >verifying that the buffer really is big enough to hold that value SHOULD
> >be what we do here, not check to see if the buffer is too big.
> >
> >Also, you can't trust that BufLength is even correct as it comes from
> >the other side, right?  Because of that, we should just be ignoring it
> >entirely and verifying that the message size really is as big as the
> >structure is supposed to be.  But that means passing down the message
> >size to the lower layers here, which gets into the issues that I have
> >raised before many years ago about this protocol, and this
> >implementation of this protocol.  I.e, it is COMPLETELY INSECURE and
> >should ONLY be used on systems where you trust both sides of the wire.
> >
> >Again, how was this change tested?  And what exactly does it fix?  I'm
> >missing how this change is going to actually catch anything, can you
> >spell it out in detail for me?
> 
> BufOffset + BufLength can exceed buffer size even if each is < RNDIS_MAX_TOTAL_SIZE

Sure, but again, BufLength is not used for anything, so the value of
that variable means nothing as far as I can tell.

> oob is triggered by a function call to gen_ndis_set_resp.

How exactly?  Again, BufLength isn't even used in that function.

> I supposed to add an additional boundry check to avoid this issue.But That 
> doesn't seem to be enough to fix the bug.I'll try to figure it out.

How was this tested?

And even more importantly, how did you find this bug?  What triggered
it?

thanks,

greg k-h