* [PATCH] usb: gadget: dfu: Fix the unchecked length field
@ 2022-11-03 4:07 Venkatesh Yadav Abbarapu
2022-11-03 15:22 ` Marek Vasut
2022-11-21 17:34 ` Tom Rini
0 siblings, 2 replies; 9+ messages in thread
From: Venkatesh Yadav Abbarapu @ 2022-11-03 4:07 UTC (permalink / raw)
To: u-boot; +Cc: michal.simek, lukma, marex, git
DFU implementation does not bound the length field in USB
DFU download setup packets, and it does not verify that
the transfer direction. Fixing the length and transfer
direction.
CVE-2022-2347
Signed-off-by: Venkatesh Yadav Abbarapu <venkatesh.abbarapu@amd.com>
---
drivers/usb/gadget/f_dfu.c | 58 ++++++++++++++++++++++++--------------
1 file changed, 37 insertions(+), 21 deletions(-)
diff --git a/drivers/usb/gadget/f_dfu.c b/drivers/usb/gadget/f_dfu.c
index e9340ff5cb..33ef62f8ba 100644
--- a/drivers/usb/gadget/f_dfu.c
+++ b/drivers/usb/gadget/f_dfu.c
@@ -321,23 +321,29 @@ static int state_dfu_idle(struct f_dfu *f_dfu,
u16 len = le16_to_cpu(ctrl->wLength);
int value = 0;
+ len = len > DFU_USB_BUFSIZ ? DFU_USB_BUFSIZ : len;
+
switch (ctrl->bRequest) {
case USB_REQ_DFU_DNLOAD:
- if (len == 0) {
- f_dfu->dfu_state = DFU_STATE_dfuERROR;
- value = RET_STALL;
- break;
+ if (ctrl->bRequestType == USB_DIR_OUT) {
+ if (len == 0) {
+ f_dfu->dfu_state = DFU_STATE_dfuERROR;
+ value = RET_STALL;
+ break;
+ }
+ f_dfu->dfu_state = DFU_STATE_dfuDNLOAD_SYNC;
+ f_dfu->blk_seq_num = w_value;
+ value = handle_dnload(gadget, len);
}
- f_dfu->dfu_state = DFU_STATE_dfuDNLOAD_SYNC;
- f_dfu->blk_seq_num = w_value;
- value = handle_dnload(gadget, len);
break;
case USB_REQ_DFU_UPLOAD:
- f_dfu->dfu_state = DFU_STATE_dfuUPLOAD_IDLE;
- f_dfu->blk_seq_num = 0;
- value = handle_upload(req, len);
- if (value >= 0 && value < len)
- f_dfu->dfu_state = DFU_STATE_dfuIDLE;
+ if (ctrl->bRequestType == USB_DIR_IN) {
+ f_dfu->dfu_state = DFU_STATE_dfuUPLOAD_IDLE;
+ f_dfu->blk_seq_num = 0;
+ value = handle_upload(req, len);
+ if (value >= 0 && value < len)
+ f_dfu->dfu_state = DFU_STATE_dfuIDLE;
+ }
break;
case USB_REQ_DFU_ABORT:
/* no zlp? */
@@ -426,11 +432,15 @@ static int state_dfu_dnload_idle(struct f_dfu *f_dfu,
u16 len = le16_to_cpu(ctrl->wLength);
int value = 0;
+ len = len > DFU_USB_BUFSIZ ? DFU_USB_BUFSIZ : len;
+
switch (ctrl->bRequest) {
case USB_REQ_DFU_DNLOAD:
- f_dfu->dfu_state = DFU_STATE_dfuDNLOAD_SYNC;
- f_dfu->blk_seq_num = w_value;
- value = handle_dnload(gadget, len);
+ if (ctrl->bRequestType == USB_DIR_OUT) {
+ f_dfu->dfu_state = DFU_STATE_dfuDNLOAD_SYNC;
+ f_dfu->blk_seq_num = w_value;
+ value = handle_dnload(gadget, len);
+ }
break;
case USB_REQ_DFU_ABORT:
f_dfu->dfu_state = DFU_STATE_dfuIDLE;
@@ -513,13 +523,17 @@ static int state_dfu_upload_idle(struct f_dfu *f_dfu,
u16 len = le16_to_cpu(ctrl->wLength);
int value = 0;
+ len = len > DFU_USB_BUFSIZ ? DFU_USB_BUFSIZ : len;
+
switch (ctrl->bRequest) {
case USB_REQ_DFU_UPLOAD:
- /* state transition if less data then requested */
- f_dfu->blk_seq_num = w_value;
- value = handle_upload(req, len);
- if (value >= 0 && value < len)
- f_dfu->dfu_state = DFU_STATE_dfuIDLE;
+ if (ctrl->bRequestType == USB_DIR_IN) {
+ /* state transition if less data then requested */
+ f_dfu->blk_seq_num = w_value;
+ value = handle_upload(req, len);
+ if (value >= 0 && value < len)
+ f_dfu->dfu_state = DFU_STATE_dfuIDLE;
+ }
break;
case USB_REQ_DFU_ABORT:
f_dfu->dfu_state = DFU_STATE_dfuIDLE;
@@ -595,6 +609,8 @@ dfu_handle(struct usb_function *f, const struct usb_ctrlrequest *ctrl)
int value = 0;
u8 req_type = ctrl->bRequestType & USB_TYPE_MASK;
+ len = len > DFU_USB_BUFSIZ ? DFU_USB_BUFSIZ : len;
+
debug("w_value: 0x%x len: 0x%x\n", w_value, len);
debug("req_type: 0x%x ctrl->bRequest: 0x%x f_dfu->dfu_state: 0x%x\n",
req_type, ctrl->bRequest, f_dfu->dfu_state);
@@ -614,7 +630,7 @@ dfu_handle(struct usb_function *f, const struct usb_ctrlrequest *ctrl)
value = dfu_state[f_dfu->dfu_state] (f_dfu, ctrl, gadget, req);
if (value >= 0) {
- req->length = value;
+ req->length = value > DFU_USB_BUFSIZ ? DFU_USB_BUFSIZ : value;
req->zero = value < len;
value = usb_ep_queue(gadget->ep0, req, 0);
if (value < 0) {
--
2.17.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] usb: gadget: dfu: Fix the unchecked length field
2022-11-03 4:07 [PATCH] usb: gadget: dfu: Fix the unchecked length field Venkatesh Yadav Abbarapu
@ 2022-11-03 15:22 ` Marek Vasut
2022-11-03 16:20 ` Tom Rini
2022-11-21 17:34 ` Tom Rini
1 sibling, 1 reply; 9+ messages in thread
From: Marek Vasut @ 2022-11-03 15:22 UTC (permalink / raw)
To: Venkatesh Yadav Abbarapu, u-boot; +Cc: michal.simek, lukma, git, Tom Rini
On 11/3/22 05:07, Venkatesh Yadav Abbarapu wrote:
> DFU implementation does not bound the length field in USB
> DFU download setup packets, and it does not verify that
> the transfer direction. Fixing the length and transfer
> direction.
>
> CVE-2022-2347
+CC Tom
Reading through https://seclists.org/oss-sec/2022/q3/41 the disclosure
timeline at the end, I am really sad that this only reached me (as the
USB maintainer) now in this form.
Maybe there should be some dedicated advertised ML for these things ?
> Signed-off-by: Venkatesh Yadav Abbarapu <venkatesh.abbarapu@amd.com>
Reviewed-by: Marek Vasut <marex@denx.de>
Tom, please pick this directly soon.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] usb: gadget: dfu: Fix the unchecked length field
2022-11-03 15:22 ` Marek Vasut
@ 2022-11-03 16:20 ` Tom Rini
0 siblings, 0 replies; 9+ messages in thread
From: Tom Rini @ 2022-11-03 16:20 UTC (permalink / raw)
To: Marek Vasut; +Cc: Venkatesh Yadav Abbarapu, u-boot, michal.simek, lukma, git
[-- Attachment #1: Type: text/plain, Size: 1396 bytes --]
On Thu, Nov 03, 2022 at 04:22:58PM +0100, Marek Vasut wrote:
> On 11/3/22 05:07, Venkatesh Yadav Abbarapu wrote:
> > DFU implementation does not bound the length field in USB
> > DFU download setup packets, and it does not verify that
> > the transfer direction. Fixing the length and transfer
> > direction.
> >
> > CVE-2022-2347
>
> +CC Tom
>
> Reading through https://seclists.org/oss-sec/2022/q3/41 the disclosure
> timeline at the end, I am really sad that this only reached me (as the USB
> maintainer) now in this form.
>
> Maybe there should be some dedicated advertised ML for these things ?
A doc/develop/security.rst would be good to have in hopes of getting the
initial inquiries out correctly (I see this one went to several wrong
places). My strong preference is to disclose things in public first as
it's unlikely malicious actors don't already know about an issue. I
don't want a list for the cases where that's not possible for other
reasons, but I'm fine with (continuing) to be the primary point of
contact for issues.
> > Signed-off-by: Venkatesh Yadav Abbarapu <venkatesh.abbarapu@amd.com>
>
> Reviewed-by: Marek Vasut <marex@denx.de>
>
> Tom, please pick this directly soon.
I see some other USB patches outstanding as well atm, I can grab this
all the same but do you want to make a USB PR with this and a few
others?
--
Tom
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] usb: gadget: dfu: Fix the unchecked length field
2022-11-03 4:07 [PATCH] usb: gadget: dfu: Fix the unchecked length field Venkatesh Yadav Abbarapu
2022-11-03 15:22 ` Marek Vasut
@ 2022-11-21 17:34 ` Tom Rini
2022-11-28 12:47 ` Marek Vasut
1 sibling, 1 reply; 9+ messages in thread
From: Tom Rini @ 2022-11-21 17:34 UTC (permalink / raw)
To: Venkatesh Yadav Abbarapu; +Cc: u-boot, michal.simek, lukma, marex, git
[-- Attachment #1: Type: text/plain, Size: 459 bytes --]
On Thu, Nov 03, 2022 at 09:37:48AM +0530, Venkatesh Yadav Abbarapu wrote:
> DFU implementation does not bound the length field in USB
> DFU download setup packets, and it does not verify that
> the transfer direction. Fixing the length and transfer
> direction.
>
> CVE-2022-2347
>
> Signed-off-by: Venkatesh Yadav Abbarapu <venkatesh.abbarapu@amd.com>
> Reviewed-by: Marek Vasut <marex@denx.de>
Applied to u-boot/master, thanks!
--
Tom
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] usb: gadget: dfu: Fix the unchecked length field
2022-11-21 17:34 ` Tom Rini
@ 2022-11-28 12:47 ` Marek Vasut
2022-11-29 19:49 ` Sultan Khan
0 siblings, 1 reply; 9+ messages in thread
From: Marek Vasut @ 2022-11-28 12:47 UTC (permalink / raw)
To: Tom Rini, Venkatesh Yadav Abbarapu; +Cc: u-boot, michal.simek, lukma, git
On 11/21/22 18:34, Tom Rini wrote:
> On Thu, Nov 03, 2022 at 09:37:48AM +0530, Venkatesh Yadav Abbarapu wrote:
>
>> DFU implementation does not bound the length field in USB
>> DFU download setup packets, and it does not verify that
>> the transfer direction. Fixing the length and transfer
>> direction.
>>
>> CVE-2022-2347
>>
>> Signed-off-by: Venkatesh Yadav Abbarapu <venkatesh.abbarapu@amd.com>
>> Reviewed-by: Marek Vasut <marex@denx.de>
>
> Applied to u-boot/master, thanks!
So this breaks DFU support in SPL as I just found out.
Any idea why ?
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] usb: gadget: dfu: Fix the unchecked length field
2022-11-28 12:47 ` Marek Vasut
@ 2022-11-29 19:49 ` Sultan Khan
2022-11-29 21:53 ` Marek Vasut
2022-11-29 23:05 ` Fabio Estevam
0 siblings, 2 replies; 9+ messages in thread
From: Sultan Khan @ 2022-11-29 19:49 UTC (permalink / raw)
To: Marek Vasut
Cc: Tom Rini, Venkatesh Yadav Abbarapu, u-boot, michal.simek, lukma, git
While I haven't yet gotten around to trying DFU with this patch applied, my
guess as to the issue would be the checks of the form "if (ctrl->
bRequestType == USB_DIR_OUT)" or "if (ctrl->bRequestType == USB_DIR_IN)".
The bRequestType field contains many flag bits other than the direction
bit. The checks should just check that the USB_DIR_IN bit (0x80) is set or
not set, rather than checking if the entire ctrl->bRequestType field equals
some value.
Sultan
On Mon, Nov 28, 2022 at 7:48 AM Marek Vasut <marex@denx.de> wrote:
> On 11/21/22 18:34, Tom Rini wrote:
> > On Thu, Nov 03, 2022 at 09:37:48AM +0530, Venkatesh Yadav Abbarapu wrote:
> >
> >> DFU implementation does not bound the length field in USB
> >> DFU download setup packets, and it does not verify that
> >> the transfer direction. Fixing the length and transfer
> >> direction.
> >>
> >> CVE-2022-2347
> >>
> >> Signed-off-by: Venkatesh Yadav Abbarapu <venkatesh.abbarapu@amd.com>
> >> Reviewed-by: Marek Vasut <marex@denx.de>
> >
> > Applied to u-boot/master, thanks!
>
> So this breaks DFU support in SPL as I just found out.
> Any idea why ?
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] usb: gadget: dfu: Fix the unchecked length field
2022-11-29 19:49 ` Sultan Khan
@ 2022-11-29 21:53 ` Marek Vasut
2022-11-29 23:05 ` Fabio Estevam
1 sibling, 0 replies; 9+ messages in thread
From: Marek Vasut @ 2022-11-29 21:53 UTC (permalink / raw)
To: Sultan Khan
Cc: Tom Rini, Venkatesh Yadav Abbarapu, u-boot, michal.simek, lukma, git
On 11/29/22 20:49, Sultan Khan wrote:
> While I haven't yet gotten around to trying DFU with this patch applied, my
> guess as to the issue would be the checks of the form "if (ctrl->
> bRequestType == USB_DIR_OUT)" or "if (ctrl->bRequestType == USB_DIR_IN)".
> The bRequestType field contains many flag bits other than the direction
> bit. The checks should just check that the USB_DIR_IN bit (0x80) is set or
> not set, rather than checking if the entire ctrl->bRequestType field equals
> some value.
>
> Sultan
I'm looking forward to a proper fix, thanks !
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] usb: gadget: dfu: Fix the unchecked length field
2022-11-29 19:49 ` Sultan Khan
2022-11-29 21:53 ` Marek Vasut
@ 2022-11-29 23:05 ` Fabio Estevam
2022-11-30 3:46 ` Sultan Khan
1 sibling, 1 reply; 9+ messages in thread
From: Fabio Estevam @ 2022-11-29 23:05 UTC (permalink / raw)
To: Sultan Khan
Cc: Marek Vasut, Tom Rini, Venkatesh Yadav Abbarapu, u-boot,
michal.simek, lukma, git
Hi Sultan,
On Tue, Nov 29, 2022 at 4:49 PM Sultan Khan <sultanqasim@gmail.com> wrote:
>
> While I haven't yet gotten around to trying DFU with this patch applied, my
> guess as to the issue would be the checks of the form "if (ctrl->
> bRequestType == USB_DIR_OUT)" or "if (ctrl->bRequestType == USB_DIR_IN)".
> The bRequestType field contains many flag bits other than the direction
> bit. The checks should just check that the USB_DIR_IN bit (0x80) is set or
> not set, rather than checking if the entire ctrl->bRequestType field equals
> some value.
Is your suggestion as below?
--- a/drivers/usb/gadget/f_dfu.c
+++ b/drivers/usb/gadget/f_dfu.c
@@ -325,7 +325,7 @@ static int state_dfu_idle(struct f_dfu *f_dfu,
switch (ctrl->bRequest) {
case USB_REQ_DFU_DNLOAD:
- if (ctrl->bRequestType == USB_DIR_OUT) {
+ if (ctrl->bRequestType & USB_DIR_OUT) {
if (len == 0) {
f_dfu->dfu_state = DFU_STATE_dfuERROR;
value = RET_STALL;
@@ -337,7 +337,7 @@ static int state_dfu_idle(struct f_dfu *f_dfu,
}
break;
case USB_REQ_DFU_UPLOAD:
- if (ctrl->bRequestType == USB_DIR_IN) {
+ if (ctrl->bRequestType & USB_DIR_IN) {
f_dfu->dfu_state = DFU_STATE_dfuUPLOAD_IDLE;
f_dfu->blk_seq_num = 0;
value = handle_upload(req, len);
@@ -436,7 +436,7 @@ static int state_dfu_dnload_idle(struct f_dfu *f_dfu,
switch (ctrl->bRequest) {
case USB_REQ_DFU_DNLOAD:
- if (ctrl->bRequestType == USB_DIR_OUT) {
+ if (ctrl->bRequestType & USB_DIR_OUT) {
f_dfu->dfu_state = DFU_STATE_dfuDNLOAD_SYNC;
f_dfu->blk_seq_num = w_value;
value = handle_dnload(gadget, len);
@@ -527,7 +527,7 @@ static int state_dfu_upload_idle(struct f_dfu *f_dfu,
switch (ctrl->bRequest) {
case USB_REQ_DFU_UPLOAD:
- if (ctrl->bRequestType == USB_DIR_IN) {
+ if (ctrl->bRequestType & USB_DIR_IN) {
/* state transition if less data then requested */
f_dfu->blk_seq_num = w_value;
value = handle_upload(req, len);
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] usb: gadget: dfu: Fix the unchecked length field
2022-11-29 23:05 ` Fabio Estevam
@ 2022-11-30 3:46 ` Sultan Khan
0 siblings, 0 replies; 9+ messages in thread
From: Sultan Khan @ 2022-11-30 3:46 UTC (permalink / raw)
To: Fabio Estevam
Cc: Marek Vasut, Tom Rini, Venkatesh Yadav Abbarapu, u-boot,
michal.simek, lukma, git
Almost, but not quite. USB_DIR_IN is 0x80, USB_DIR_OUT is just 0, so testing (ctrl->bRequestType & USB_DIR_OUT) is meaningless. Instead, the test for an OUT transfer should be !(ctrl->bRequestType & USB_DIR_IN).
Sultan
> On Nov 29, 2022, at 6:05 PM, Fabio Estevam <festevam@gmail.com> wrote:
>
> Hi Sultan,
>
> On Tue, Nov 29, 2022 at 4:49 PM Sultan Khan <sultanqasim@gmail.com> wrote:
>>
>> While I haven't yet gotten around to trying DFU with this patch applied, my
>> guess as to the issue would be the checks of the form "if (ctrl->
>> bRequestType == USB_DIR_OUT)" or "if (ctrl->bRequestType == USB_DIR_IN)".
>> The bRequestType field contains many flag bits other than the direction
>> bit. The checks should just check that the USB_DIR_IN bit (0x80) is set or
>> not set, rather than checking if the entire ctrl->bRequestType field equals
>> some value.
>
> Is your suggestion as below?
>
> --- a/drivers/usb/gadget/f_dfu.c
> +++ b/drivers/usb/gadget/f_dfu.c
> @@ -325,7 +325,7 @@ static int state_dfu_idle(struct f_dfu *f_dfu,
>
> switch (ctrl->bRequest) {
> case USB_REQ_DFU_DNLOAD:
> - if (ctrl->bRequestType == USB_DIR_OUT) {
> + if (ctrl->bRequestType & USB_DIR_OUT) {
> if (len == 0) {
> f_dfu->dfu_state = DFU_STATE_dfuERROR;
> value = RET_STALL;
> @@ -337,7 +337,7 @@ static int state_dfu_idle(struct f_dfu *f_dfu,
> }
> break;
> case USB_REQ_DFU_UPLOAD:
> - if (ctrl->bRequestType == USB_DIR_IN) {
> + if (ctrl->bRequestType & USB_DIR_IN) {
> f_dfu->dfu_state = DFU_STATE_dfuUPLOAD_IDLE;
> f_dfu->blk_seq_num = 0;
> value = handle_upload(req, len);
> @@ -436,7 +436,7 @@ static int state_dfu_dnload_idle(struct f_dfu *f_dfu,
>
> switch (ctrl->bRequest) {
> case USB_REQ_DFU_DNLOAD:
> - if (ctrl->bRequestType == USB_DIR_OUT) {
> + if (ctrl->bRequestType & USB_DIR_OUT) {
> f_dfu->dfu_state = DFU_STATE_dfuDNLOAD_SYNC;
> f_dfu->blk_seq_num = w_value;
> value = handle_dnload(gadget, len);
> @@ -527,7 +527,7 @@ static int state_dfu_upload_idle(struct f_dfu *f_dfu,
>
> switch (ctrl->bRequest) {
> case USB_REQ_DFU_UPLOAD:
> - if (ctrl->bRequestType == USB_DIR_IN) {
> + if (ctrl->bRequestType & USB_DIR_IN) {
> /* state transition if less data then requested */
> f_dfu->blk_seq_num = w_value;
> value = handle_upload(req, len);
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2022-11-30 3:46 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-11-03 4:07 [PATCH] usb: gadget: dfu: Fix the unchecked length field Venkatesh Yadav Abbarapu
2022-11-03 15:22 ` Marek Vasut
2022-11-03 16:20 ` Tom Rini
2022-11-21 17:34 ` Tom Rini
2022-11-28 12:47 ` Marek Vasut
2022-11-29 19:49 ` Sultan Khan
2022-11-29 21:53 ` Marek Vasut
2022-11-29 23:05 ` Fabio Estevam
2022-11-30 3:46 ` Sultan Khan
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.