mv_u3d_req_to_trb() does not check for dma mapping errors.
By the way, the patch improves readability of mv_u3d_start_queue()
by rearranging its code with two semantic modifications:
- assignment zero to ep->processing if usb_gadget_map_request() fails;
- propagation of error code from mv_u3d_req_to_trb() instead of
hardcoded -ENOMEM.
Found by Linux Driver Verification project (linuxtesting.org).
Signed-off-by: Alexey Khoroshilov <[email protected]>
---
drivers/usb/gadget/udc/mv_u3d_core.c | 34 +++++++++++++++++++++-------------
1 file changed, 21 insertions(+), 13 deletions(-)
diff --git a/drivers/usb/gadget/udc/mv_u3d_core.c b/drivers/usb/gadget/udc/mv_u3d_core.c
index b9e19a591322..8d726bd767fd 100644
--- a/drivers/usb/gadget/udc/mv_u3d_core.c
+++ b/drivers/usb/gadget/udc/mv_u3d_core.c
@@ -462,6 +462,12 @@ static int mv_u3d_req_to_trb(struct mv_u3d_req *req)
req->trb_head->trb_hw,
trb_num * sizeof(*trb_hw),
DMA_BIDIRECTIONAL);
+ if (dma_mapping_error(u3d->gadget.dev.parent,
+ req->trb_head->trb_dma)) {
+ kfree(req->trb_head->trb_hw);
+ kfree(req->trb_head);
+ return -EFAULT;
+ }
req->chain = 1;
}
@@ -487,30 +493,32 @@ mv_u3d_start_queue(struct mv_u3d_ep *ep)
ret = usb_gadget_map_request(&u3d->gadget, &req->req,
mv_u3d_ep_dir(ep));
if (ret)
- return ret;
+ goto break_processing;
req->req.status = -EINPROGRESS;
req->req.actual = 0;
req->trb_count = 0;
- /* build trbs and push them to device queue */
- if (!mv_u3d_req_to_trb(req)) {
- ret = mv_u3d_queue_trb(ep, req);
- if (ret) {
- ep->processing = 0;
- return ret;
- }
- } else {
- ep->processing = 0;
+ /* build trbs */
+ ret = mv_u3d_req_to_trb(req);
+ if (ret) {
dev_err(u3d->dev, "%s, mv_u3d_req_to_trb fail\n", __func__);
- return -ENOMEM;
+ goto break_processing;
}
+ /* and push them to device queue */
+ ret = mv_u3d_queue_trb(ep, req);
+ if (ret)
+ goto break_processing;
+
/* irq handler advances the queue */
- if (req)
- list_add_tail(&req->queue, &ep->queue);
+ list_add_tail(&req->queue, &ep->queue);
return 0;
+
+break_processing:
+ ep->processing = 0;
+ return ret;
}
static int mv_u3d_ep_enable(struct usb_ep *_ep,
--
2.7.4
Hi,
Alexey Khoroshilov <[email protected]> writes:
> mv_u3d_req_to_trb() does not check for dma mapping errors.
>
> By the way, the patch improves readability of mv_u3d_start_queue()
> by rearranging its code with two semantic modifications:
> - assignment zero to ep->processing if usb_gadget_map_request() fails;
> - propagation of error code from mv_u3d_req_to_trb() instead of
> hardcoded -ENOMEM.
cleanups and fixes should be done separately.
> Found by Linux Driver Verification project (linuxtesting.org).
>
> Signed-off-by: Alexey Khoroshilov <[email protected]>
> ---
> drivers/usb/gadget/udc/mv_u3d_core.c | 34 +++++++++++++++++++++-------------
> 1 file changed, 21 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/usb/gadget/udc/mv_u3d_core.c b/drivers/usb/gadget/udc/mv_u3d_core.c
> index b9e19a591322..8d726bd767fd 100644
> --- a/drivers/usb/gadget/udc/mv_u3d_core.c
> +++ b/drivers/usb/gadget/udc/mv_u3d_core.c
> @@ -462,6 +462,12 @@ static int mv_u3d_req_to_trb(struct mv_u3d_req *req)
> req->trb_head->trb_hw,
> trb_num * sizeof(*trb_hw),
> DMA_BIDIRECTIONAL);
> + if (dma_mapping_error(u3d->gadget.dev.parent,
> + req->trb_head->trb_dma)) {
> + kfree(req->trb_head->trb_hw);
> + kfree(req->trb_head);
> + return -EFAULT;
> + }
>
> req->chain = 1;
> }
this is one patch: add dma_mapping_error() check
AKA $subject :-p
> @@ -487,30 +493,32 @@ mv_u3d_start_queue(struct mv_u3d_ep *ep)
> ret = usb_gadget_map_request(&u3d->gadget, &req->req,
> mv_u3d_ep_dir(ep));
> if (ret)
> - return ret;
> + goto break_processing;
>
> req->req.status = -EINPROGRESS;
> req->req.actual = 0;
> req->trb_count = 0;
>
> - /* build trbs and push them to device queue */
> - if (!mv_u3d_req_to_trb(req)) {
> - ret = mv_u3d_queue_trb(ep, req);
> - if (ret) {
> - ep->processing = 0;
> - return ret;
> - }
> - } else {
> - ep->processing = 0;
> + /* build trbs */
> + ret = mv_u3d_req_to_trb(req);
> + if (ret) {
> dev_err(u3d->dev, "%s, mv_u3d_req_to_trb fail\n", __func__);
> - return -ENOMEM;
> + goto break_processing;
> }
>
> + /* and push them to device queue */
> + ret = mv_u3d_queue_trb(ep, req);
> + if (ret)
> + goto break_processing;
> +
> /* irq handler advances the queue */
> - if (req)
> - list_add_tail(&req->queue, &ep->queue);
> + list_add_tail(&req->queue, &ep->queue);
>
> return 0;
> +
> +break_processing:
> + ep->processing = 0;
> + return ret;
> }
>
> static int mv_u3d_ep_enable(struct usb_ep *_ep,
this is another, unrelated patch. Please split
--
balbi
mv_u3d_req_to_trb() does not check for dma mapping errors.
Found by Linux Driver Verification project (linuxtesting.org).
v2: split fix and clenup to separate patches.
Signed-off-by: Alexey Khoroshilov <[email protected]>
---
drivers/usb/gadget/udc/mv_u3d_core.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/drivers/usb/gadget/udc/mv_u3d_core.c b/drivers/usb/gadget/udc/mv_u3d_core.c
index b9e19a591322..6f3be0ba9ac8 100644
--- a/drivers/usb/gadget/udc/mv_u3d_core.c
+++ b/drivers/usb/gadget/udc/mv_u3d_core.c
@@ -462,6 +462,12 @@ static int mv_u3d_req_to_trb(struct mv_u3d_req *req)
req->trb_head->trb_hw,
trb_num * sizeof(*trb_hw),
DMA_BIDIRECTIONAL);
+ if (dma_mapping_error(u3d->gadget.dev.parent,
+ req->trb_head->trb_dma)) {
+ kfree(req->trb_head->trb_hw);
+ kfree(req->trb_head);
+ return -EFAULT;
+ }
req->chain = 1;
}
--
2.7.4
The patch improves readability of mv_u3d_start_queue()
by rearranging its code with two semantic modifications:
- assignment zero to ep->processing if usb_gadget_map_request() fails;
- propagation of error code from mv_u3d_req_to_trb() instead of
hardcoded -ENOMEM.
Signed-off-by: Alexey Khoroshilov <[email protected]>
---
drivers/usb/gadget/udc/mv_u3d_core.c | 28 +++++++++++++++-------------
1 file changed, 15 insertions(+), 13 deletions(-)
diff --git a/drivers/usb/gadget/udc/mv_u3d_core.c b/drivers/usb/gadget/udc/mv_u3d_core.c
index 6f3be0ba9ac8..8d726bd767fd 100644
--- a/drivers/usb/gadget/udc/mv_u3d_core.c
+++ b/drivers/usb/gadget/udc/mv_u3d_core.c
@@ -493,30 +493,32 @@ mv_u3d_start_queue(struct mv_u3d_ep *ep)
ret = usb_gadget_map_request(&u3d->gadget, &req->req,
mv_u3d_ep_dir(ep));
if (ret)
- return ret;
+ goto break_processing;
req->req.status = -EINPROGRESS;
req->req.actual = 0;
req->trb_count = 0;
- /* build trbs and push them to device queue */
- if (!mv_u3d_req_to_trb(req)) {
- ret = mv_u3d_queue_trb(ep, req);
- if (ret) {
- ep->processing = 0;
- return ret;
- }
- } else {
- ep->processing = 0;
+ /* build trbs */
+ ret = mv_u3d_req_to_trb(req);
+ if (ret) {
dev_err(u3d->dev, "%s, mv_u3d_req_to_trb fail\n", __func__);
- return -ENOMEM;
+ goto break_processing;
}
+ /* and push them to device queue */
+ ret = mv_u3d_queue_trb(ep, req);
+ if (ret)
+ goto break_processing;
+
/* irq handler advances the queue */
- if (req)
- list_add_tail(&req->queue, &ep->queue);
+ list_add_tail(&req->queue, &ep->queue);
return 0;
+
+break_processing:
+ ep->processing = 0;
+ return ret;
}
static int mv_u3d_ep_enable(struct usb_ep *_ep,
--
2.7.4
Hi,
Alexey Khoroshilov <[email protected]> writes:
> mv_u3d_req_to_trb() does not check for dma mapping errors.
>
> Found by Linux Driver Verification project (linuxtesting.org).
>
> v2: split fix and clenup to separate patches.
I'll fix this time when applying, but keep in mind we don't want these
notes in the commit log. They should come after the tearline (---)
below, together with the diffstat ;-)
> Signed-off-by: Alexey Khoroshilov <[email protected]>
> ---
> drivers/usb/gadget/udc/mv_u3d_core.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/drivers/usb/gadget/udc/mv_u3d_core.c b/drivers/usb/gadget/udc/mv_u3d_core.c
> index b9e19a591322..6f3be0ba9ac8 100644
> --- a/drivers/usb/gadget/udc/mv_u3d_core.c
> +++ b/drivers/usb/gadget/udc/mv_u3d_core.c
> @@ -462,6 +462,12 @@ static int mv_u3d_req_to_trb(struct mv_u3d_req *req)
> req->trb_head->trb_hw,
> trb_num * sizeof(*trb_hw),
> DMA_BIDIRECTIONAL);
> + if (dma_mapping_error(u3d->gadget.dev.parent,
> + req->trb_head->trb_dma)) {
> + kfree(req->trb_head->trb_hw);
> + kfree(req->trb_head);
> + return -EFAULT;
> + }
>
> req->chain = 1;
> }
> --
> 2.7.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
balbi
On 03.11.2016 16:34, Felipe Balbi wrote:
>
> Hi,
>
> Alexey Khoroshilov <[email protected]> writes:
>> mv_u3d_req_to_trb() does not check for dma mapping errors.
>>
>> Found by Linux Driver Verification project (linuxtesting.org).
>>
>> v2: split fix and clenup to separate patches.
>
> I'll fix this time when applying, but keep in mind we don't want these
> notes in the commit log. They should come after the tearline (---)
> below, together with the diffstat ;-)
ok, thank you
--
Alexey