Re: [PATCH] usb: gadget: dfu: Fix the unchecked length field
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 wrote:
>
> Hi Sultan,
>
> On Tue, Nov 29, 2022 at 4:49 PM 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.
>
> 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);
Re: [PATCH] usb: gadget: dfu: Fix the unchecked length field
Hi Sultan,
On Tue, Nov 29, 2022 at 4:49 PM 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.
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);
Re: [PATCH] usb: gadget: dfu: Fix the unchecked length field
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 !
Re: [PATCH] usb: gadget: dfu: Fix the unchecked length field
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 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 > >> Reviewed-by: Marek Vasut > > > > Applied to u-boot/master, thanks! > > So this breaks DFU support in SPL as I just found out. > Any idea why ? >
Re: [PATCH] usb: gadget: dfu: Fix the unchecked length field
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 Reviewed-by: Marek Vasut Applied to u-boot/master, thanks! So this breaks DFU support in SPL as I just found out. Any idea why ?
Re: [PATCH] usb: gadget: dfu: Fix the unchecked length field
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 > Reviewed-by: Marek Vasut Applied to u-boot/master, thanks! -- Tom signature.asc Description: PGP signature
Re: [PATCH] usb: gadget: dfu: Fix the unchecked length field
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 > > Reviewed-by: Marek Vasut > > 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 signature.asc Description: PGP signature
Re: [PATCH] usb: gadget: dfu: Fix the unchecked length field
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 Reviewed-by: Marek Vasut Tom, please pick this directly soon.
