Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
Content-Type: text/plain;
        charset=us-ascii
Mime-Version: 1.0 (Mac OS X Mail 15.0 \(3693.60.0.1.1\))
Subject: Re: [RFC PATCH 03/13] usb: remove the usage of the list iterator
 after the loop
From:   Jakob <jakobkoschel@gmail.com>
In-Reply-To: <6DFD3D91-B82C-469C-8771-860C09BD8623@gmail.com>
Date:   Wed, 23 Feb 2022 15:16:09 +0100
Cc:     Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        linux-arch <linux-arch@vger.kernel.org>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        Arnd Bergman <arnd@arndb.de>,
        Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        Kees Cook <keescook@chromium.org>,
        Mike Rapoport <rppt@kernel.org>,
        "Gustavo A. R. Silva" <gustavo@embeddedor.com>,
        Brian Johannesmeyer <bjohannesmeyer@gmail.com>,
        Cristiano Giuffrida <c.giuffrida@vu.nl>,
        "Bos, H.J." <h.j.bos@vu.nl>
Content-Transfer-Encoding: quoted-printable
Message-Id: <86C4CE7D-6D93-456B-AA82-F8ADEACA40B7@gmail.com>
References: <20220217184829.1991035-1-jakobkoschel@gmail.com>
 <20220217184829.1991035-4-jakobkoschel@gmail.com>
 <CAHk-=wg1RdFQ6OGb_H4ZJoUwEr-gk11QXeQx63n91m0tvVUdZw@mail.gmail.com>
 <6DFD3D91-B82C-469C-8771-860C09BD8623@gmail.com>
To:     Linus Torvalds <torvalds@linux-foundation.org>
Precedence: bulk

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 =3D list_first_entry(head, typeof(*pos), member);

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

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

Since the list is empty head->next =3D=3D 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@gmail.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 =3D -EINVAL;
		goto out;
	}
+	printk(KERN_INFO "JKL: This does print, but shouldn't");

	if (list_first_entry(&ep->queue, struct gr_request, queue) =3D=3D =
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 =3D kzalloc(sizeof(struct gr_ep)+sizeof(struct =
usb_request), GFP_KERNEL);
+	ep =3D buffer;
+	_req =3D buffer+sizeof(struct gr_ep);
+
+	/* setup to call gr_dequeue() */
+	dev =3D kzalloc(sizeof(struct gr_udc), GFP_KERNEL);
+	ep->dev =3D dev;
+	ep->dev->driver =3D 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 {
--=20
2.25.1