[PATCH] usb: gadget: dfu: Fix the unchecked length field
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) {
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.
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?
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!
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 ?
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 ?
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 !
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);
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);
participants (5)
-
Fabio Estevam -
Marek Vasut -
Sultan Khan -
Tom Rini -
Venkatesh Yadav Abbarapu