linux-kernel - Re: [RFC PATCH 03/13] usb: remove the usage of the list iterator after the loop

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]

Date:   Wed, 23 Feb 2022 15:16:09 +0100
From:   Jakob <jakobkoschel@...il.com>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        linux-arch <linux-arch@...r.kernel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Arnd Bergman <arnd@...db.de>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Kees Cook <keescook@...omium.org>,
        Mike Rapoport <rppt@...nel.org>,
        "Gustavo A. R. Silva" <gustavo@...eddedor.com>,
        Brian Johannesmeyer <bjohannesmeyer@...il.com>,
        Cristiano Giuffrida <c.giuffrida@...nl>,
        "Bos, H.J." <h.j.bos@...nl>
Subject: Re: [RFC PATCH 03/13] usb: remove the usage of the list iterator
 after the loop

Note that I changed the location of the struct member 'req' in gr_request
to make this work. Instead of reshuffling struct members this can also be
introduced by simply adding new struct members in certain spots.
In other code locations with the same pattern I didn't have to do that.

Assuming '_req' passed to gr_dequeue() is located just past 'ep' on the
heap, the check could pass even though the list searched is completely
empty.

&req->req for the head element will be an out-of-bounds pointer
that by coincidence or heap massaging is where '_req' is located.

Even if the list is empty the list_for_each_entry() macro will do:

	pos = list_first_entry(head, typeof(*pos), member);

resolving all the macros (list_first_entry() etc) it will look like this:

	pos = container_of(head->next, typeof(*pos), member)

Since the list is empty head->next == head and container_of() is called on something
that is *not* actually of type gr_request.

Next, the check if the end of the loop is hit is evaluated:

	!list_entry_is_head(pos, head, member);

which will stop the loop directly before executing a single iteration.

then using '&req->req' is some bogus pointer pointing just past the struct gr_ep,
where in this case '_req' is located.

The point I'm trying to make: it's probably not safe to rely on the compiler and
that everyone is aware of this risk when adding/removing/reordering struct members.


Signed-off-by: Jakob Koschel <jakobkoschel@...il.com>
---
drivers/usb/gadget/udc/gr_udc.c | 25 +++++++++++++++++++++++++
drivers/usb/gadget/udc/gr_udc.h |  2 +-
2 files changed, 26 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/gadget/udc/gr_udc.c b/drivers/usb/gadget/udc/gr_udc.c
index 4b35739d3695..29c662f28428 100644
--- a/drivers/usb/gadget/udc/gr_udc.c
+++ b/drivers/usb/gadget/udc/gr_udc.c
@@ -1718,6 +1718,7 @@ static int gr_dequeue(struct usb_ep *_ep, struct usb_request *_req)
		ret = -EINVAL;
		goto out;
	}
+	printk(KERN_INFO "JKL: This does print, but shouldn't");

	if (list_first_entry(&ep->queue, struct gr_request, queue) == req) {
		/* This request is currently being processed */
@@ -1739,6 +1740,30 @@ static int gr_dequeue(struct usb_ep *_ep, struct usb_request *_req)
	return ret;
}

+static int __init init_test_jkl3(void)
+{
+	struct gr_ep *ep;
+	struct gr_udc *dev;
+	struct usb_request *_req;
+	void *buffer;
+
+	/* assume the usb_request struct is located just after the 'ep' on the heap */
+	buffer = kzalloc(sizeof(struct gr_ep)+sizeof(struct usb_request), GFP_KERNEL);
+	ep = buffer;
+	_req = buffer+sizeof(struct gr_ep);
+
+	/* setup to call gr_dequeue() */
+	dev = kzalloc(sizeof(struct gr_udc), GFP_KERNEL);
+	ep->dev = dev;
+	ep->dev->driver = 1;
+	INIT_LIST_HEAD(&ep->queue); /* list is empty */
+
+	gr_dequeue(&ep->ep, _req);
+
+	return 0;
+}
+__initcall(init_test_jkl3);
+
/* Helper for gr_set_halt and gr_set_wedge */
static int gr_set_halt_wedge(struct usb_ep *_ep, int halt, int wedge)
{
diff --git a/drivers/usb/gadget/udc/gr_udc.h b/drivers/usb/gadget/udc/gr_udc.h
index 70134239179e..14a18d5b5cf8 100644
--- a/drivers/usb/gadget/udc/gr_udc.h
+++ b/drivers/usb/gadget/udc/gr_udc.h
@@ -159,7 +159,6 @@ struct gr_ep {
};

struct gr_request {
-	struct usb_request req;
	struct list_head queue;

	/* Chain of dma descriptors */
@@ -171,6 +170,7 @@ struct gr_request {
	u16 oddlen; /* Size of odd length tail if buffer length is "odd" */

	u8 setup; /* Setup packet */
+	struct usb_request req;
};

enum gr_ep0state {
-- 
2.25.1