[PATCH] usb: gadget: dfu: Fix the unchecked length field

[Sultan Khan]

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 at gmail.com> wrote:
> 
> Hi Sultan,
> 
> On Tue, Nov 29, 2022 at 4:49 PM Sultan Khan <sultanqasim at 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);