| Message ID | 20200530032400.12743-1-baijiaju1990@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | usb: cdns3: fix possible buffer overflow caused by bad DMA value | expand |
On 20-05-30 11:24:00, Jia-Ju Bai wrote: > In cdns3_ep0_setup_phase(): > struct usb_ctrlrequest *ctrl = priv_dev->setup_buf; > > Because priv_dev->setup_buf (allocated in cdns3_gadget_start) is stored > in DMA memory, and thus ctrl is a DMA value. > > cdns3_ep0_setup_phase() > cdns3_ep0_standard_request(priv_dev, ctrl) > cdns3_req_ep0_get_status(priv_dev, ctrl) > index = cdns3_ep_addr_to_index(ctrl->wIndex); > priv_ep = priv_dev->eps[index]; > > cdns3_ep0_setup_phase() > cdns3_ep0_standard_request(priv_dev, ctrl) > cdns3_req_ep0_handle_feature(priv_dev, ctrl_req, 0) > cdns3_ep0_feature_handle_endpoint(priv_dev, ctrl, set) > index = cdns3_ep_addr_to_index(ctrl->wIndex); > priv_ep = priv_dev->eps[index]; > > In these cases, ctrl->wIndex can be be modified at anytime by malicious > hardware, and thus a buffer overflow can occur when the code > "priv_dev->eps[index]" is executed. > Did you see the setup buffer is overwritten before the setup handling is finished? > To fix these possible bugs, index is checked before being used. I think the better fix is to use one additional buffer for struct usb_ctrlrequest, and copy the dma_buf to it after setup packet has received, then use the value stored in this buffer for later operation, it could avoid quitting the code which is useful in fact. Peter > > Signed-off-by: Jia-Ju Bai <baijiaju1990@gmail.com> > --- > drivers/usb/cdns3/ep0.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/drivers/usb/cdns3/ep0.c b/drivers/usb/cdns3/ep0.c > index e71240b386b4..0a80c7ade613 100644 > --- a/drivers/usb/cdns3/ep0.c > +++ b/drivers/usb/cdns3/ep0.c > @@ -265,6 +265,8 @@ static int cdns3_req_ep0_get_status(struct cdns3_device *priv_dev, > return cdns3_ep0_delegate_req(priv_dev, ctrl); > case USB_RECIP_ENDPOINT: > index = cdns3_ep_addr_to_index(ctrl->wIndex); > + if (index >= CDNS3_ENDPOINTS_MAX_COUNT) > + return -EINVAL; > priv_ep = priv_dev->eps[index]; > > /* check if endpoint is stalled or stall is pending */ > @@ -388,6 +390,9 @@ static int cdns3_ep0_feature_handle_endpoint(struct cdns3_device *priv_dev, > return 0; > > index = cdns3_ep_addr_to_index(ctrl->wIndex); > + if (index >= CDNS3_ENDPOINTS_MAX_COUNT) > + return -EINVAL; > + > priv_ep = priv_dev->eps[index]; > > cdns3_select_ep(priv_dev, ctrl->wIndex); > -- > 2.17.1 >
Peter Chen <peter.chen@nxp.com> writes: > On 20-05-30 11:24:00, Jia-Ju Bai wrote: >> In cdns3_ep0_setup_phase(): >> struct usb_ctrlrequest *ctrl = priv_dev->setup_buf; >> >> Because priv_dev->setup_buf (allocated in cdns3_gadget_start) is stored >> in DMA memory, and thus ctrl is a DMA value. >> >> cdns3_ep0_setup_phase() >> cdns3_ep0_standard_request(priv_dev, ctrl) >> cdns3_req_ep0_get_status(priv_dev, ctrl) >> index = cdns3_ep_addr_to_index(ctrl->wIndex); >> priv_ep = priv_dev->eps[index]; >> >> cdns3_ep0_setup_phase() >> cdns3_ep0_standard_request(priv_dev, ctrl) >> cdns3_req_ep0_handle_feature(priv_dev, ctrl_req, 0) >> cdns3_ep0_feature_handle_endpoint(priv_dev, ctrl, set) >> index = cdns3_ep_addr_to_index(ctrl->wIndex); >> priv_ep = priv_dev->eps[index]; >> >> In these cases, ctrl->wIndex can be be modified at anytime by malicious >> hardware, and thus a buffer overflow can occur when the code >> "priv_dev->eps[index]" is executed. >> > > Did you see the setup buffer is overwritten before the setup handling is > finished? > >> To fix these possible bugs, index is checked before being used. > > I think the better fix is to use one additional buffer for struct > usb_ctrlrequest, and copy the dma_buf to it after setup packet > has received, then use the value stored in this buffer for later > operation, it could avoid quitting the code which is useful in fact. Why is this a better fix? If you don't have that endpoint index, you shouldn't try to access it. However, I think the problem here is slightly easier to solve :-) >> diff --git a/drivers/usb/cdns3/ep0.c b/drivers/usb/cdns3/ep0.c >> index e71240b386b4..0a80c7ade613 100644 >> --- a/drivers/usb/cdns3/ep0.c >> +++ b/drivers/usb/cdns3/ep0.c >> @@ -265,6 +265,8 @@ static int cdns3_req_ep0_get_status(struct cdns3_device *priv_dev, >> return cdns3_ep0_delegate_req(priv_dev, ctrl); >> case USB_RECIP_ENDPOINT: >> index = cdns3_ep_addr_to_index(ctrl->wIndex); diff --git a/drivers/usb/cdns3/gadget.c b/drivers/usb/cdns3/gadget.c index 5e24c2e57c0d..96ba3eec805c 100644 --- a/drivers/usb/cdns3/gadget.c +++ b/drivers/usb/cdns3/gadget.c @@ -107,7 +107,10 @@ void cdns3_set_register_bit(void __iomem *ptr, u32 mask) */ u8 cdns3_ep_addr_to_index(u8 ep_addr) { - return (((ep_addr & 0x7F)) + ((ep_addr & USB_DIR_IN) ? 16 : 0)); + u8 num = ep_addr & USB_ENDPOINT_NUMBER_MASK; + u8 dir = ep_addr & USB_ENDPOINT_DIR_MASK; + + return num + dir ? 16 : 0; } static int cdns3_get_dma_pos(struct cdns3_device *priv_dev, This will guarantee that the number is never over the limit.
On 20-07-01 09:52:43, Felipe Balbi wrote: > Peter Chen <peter.chen@nxp.com> writes: > > > On 20-05-30 11:24:00, Jia-Ju Bai wrote: > >> In cdns3_ep0_setup_phase(): > >> struct usb_ctrlrequest *ctrl = priv_dev->setup_buf; > >> > >> Because priv_dev->setup_buf (allocated in cdns3_gadget_start) is stored > >> in DMA memory, and thus ctrl is a DMA value. > >> > >> cdns3_ep0_setup_phase() > >> cdns3_ep0_standard_request(priv_dev, ctrl) > >> cdns3_req_ep0_get_status(priv_dev, ctrl) > >> index = cdns3_ep_addr_to_index(ctrl->wIndex); > >> priv_ep = priv_dev->eps[index]; > >> > >> cdns3_ep0_setup_phase() > >> cdns3_ep0_standard_request(priv_dev, ctrl) > >> cdns3_req_ep0_handle_feature(priv_dev, ctrl_req, 0) > >> cdns3_ep0_feature_handle_endpoint(priv_dev, ctrl, set) > >> index = cdns3_ep_addr_to_index(ctrl->wIndex); > >> priv_ep = priv_dev->eps[index]; > >> > >> In these cases, ctrl->wIndex can be be modified at anytime by malicious > >> hardware, and thus a buffer overflow can occur when the code > >> "priv_dev->eps[index]" is executed. > >> > > > > Did you see the setup buffer is overwritten before the setup handling is > > finished? > > > >> To fix these possible bugs, index is checked before being used. > > > > I think the better fix is to use one additional buffer for struct > > usb_ctrlrequest, and copy the dma_buf to it after setup packet > > has received, then use the value stored in this buffer for later > > operation, it could avoid quitting the code which is useful in fact. > > Why is this a better fix? If you don't have that endpoint index, you > shouldn't try to access it. However, I think the problem here is > slightly easier to solve :-) The possible problem here is: it is a correct setup packet, the memory it uses may be modified by controller wrongly (eg, try to get next setup packet) before it finishes using. So, I suggest adding a setup buf for struct cdns3 to store every setup packet after it receives to avoid the original setup buffer corrupted. Peter > > >> diff --git a/drivers/usb/cdns3/ep0.c b/drivers/usb/cdns3/ep0.c > >> index e71240b386b4..0a80c7ade613 100644 > >> --- a/drivers/usb/cdns3/ep0.c > >> +++ b/drivers/usb/cdns3/ep0.c > >> @@ -265,6 +265,8 @@ static int cdns3_req_ep0_get_status(struct cdns3_device *priv_dev, > >> return cdns3_ep0_delegate_req(priv_dev, ctrl); > >> case USB_RECIP_ENDPOINT: > >> index = cdns3_ep_addr_to_index(ctrl->wIndex); > > diff --git a/drivers/usb/cdns3/gadget.c b/drivers/usb/cdns3/gadget.c > index 5e24c2e57c0d..96ba3eec805c 100644 > --- a/drivers/usb/cdns3/gadget.c > +++ b/drivers/usb/cdns3/gadget.c > @@ -107,7 +107,10 @@ void cdns3_set_register_bit(void __iomem *ptr, u32 mask) > */ > u8 cdns3_ep_addr_to_index(u8 ep_addr) > { > - return (((ep_addr & 0x7F)) + ((ep_addr & USB_DIR_IN) ? 16 : 0)); > + u8 num = ep_addr & USB_ENDPOINT_NUMBER_MASK; > + u8 dir = ep_addr & USB_ENDPOINT_DIR_MASK; > + > + return num + dir ? 16 : 0; > } > > static int cdns3_get_dma_pos(struct cdns3_device *priv_dev, > > This will guarantee that the number is never over the limit. > > -- > balbi
diff --git a/drivers/usb/cdns3/ep0.c b/drivers/usb/cdns3/ep0.c index e71240b386b4..0a80c7ade613 100644 --- a/drivers/usb/cdns3/ep0.c +++ b/drivers/usb/cdns3/ep0.c @@ -265,6 +265,8 @@ static int cdns3_req_ep0_get_status(struct cdns3_device *priv_dev, return cdns3_ep0_delegate_req(priv_dev, ctrl); case USB_RECIP_ENDPOINT: index = cdns3_ep_addr_to_index(ctrl->wIndex); + if (index >= CDNS3_ENDPOINTS_MAX_COUNT) + return -EINVAL; priv_ep = priv_dev->eps[index]; /* check if endpoint is stalled or stall is pending */ @@ -388,6 +390,9 @@ static int cdns3_ep0_feature_handle_endpoint(struct cdns3_device *priv_dev, return 0; index = cdns3_ep_addr_to_index(ctrl->wIndex); + if (index >= CDNS3_ENDPOINTS_MAX_COUNT) + return -EINVAL; + priv_ep = priv_dev->eps[index]; cdns3_select_ep(priv_dev, ctrl->wIndex);
In cdns3_ep0_setup_phase(): struct usb_ctrlrequest *ctrl = priv_dev->setup_buf; Because priv_dev->setup_buf (allocated in cdns3_gadget_start) is stored in DMA memory, and thus ctrl is a DMA value. cdns3_ep0_setup_phase() cdns3_ep0_standard_request(priv_dev, ctrl) cdns3_req_ep0_get_status(priv_dev, ctrl) index = cdns3_ep_addr_to_index(ctrl->wIndex); priv_ep = priv_dev->eps[index]; cdns3_ep0_setup_phase() cdns3_ep0_standard_request(priv_dev, ctrl) cdns3_req_ep0_handle_feature(priv_dev, ctrl_req, 0) cdns3_ep0_feature_handle_endpoint(priv_dev, ctrl, set) index = cdns3_ep_addr_to_index(ctrl->wIndex); priv_ep = priv_dev->eps[index]; In these cases, ctrl->wIndex can be be modified at anytime by malicious hardware, and thus a buffer overflow can occur when the code "priv_dev->eps[index]" is executed. To fix these possible bugs, index is checked before being used. Signed-off-by: Jia-Ju Bai <baijiaju1990@gmail.com> --- drivers/usb/cdns3/ep0.c | 5 +++++ 1 file changed, 5 insertions(+)