* [PATCH] USB: whci-hcd: add more checks for dma mapping error @ 2016-03-25 20:23 Alexey Khoroshilov 2016-03-25 22:03 ` Vladimir Zapolskiy 0 siblings, 1 reply; 6+ messages in thread From: Alexey Khoroshilov @ 2016-03-25 20:23 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Alexey Khoroshilov, linux-usb, linux-kernel, ldv-project Fixing checks for dma mapping error in qset_fill_page_list(), I have missed two similar problems in qset_add_urb_sg() and in qset_add_urb_sg_linearize(). Found by Linux Driver Verification project (linuxtesting.org). Signed-off-by: Alexey Khoroshilov <khoroshilov@ispras.ru> --- drivers/usb/host/whci/qset.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/drivers/usb/host/whci/qset.c b/drivers/usb/host/whci/qset.c index 1a8e960d073b..a8e9b618e643 100644 --- a/drivers/usb/host/whci/qset.c +++ b/drivers/usb/host/whci/qset.c @@ -535,9 +535,11 @@ static int qset_add_urb_sg(struct whc *whc, struct whc_qset *qset, struct urb *u list_for_each_entry(std, &qset->stds, list_node) { if (std->ntds_remaining == -1) { pl_len = std->num_pointers * sizeof(struct whc_page_list_entry); - std->ntds_remaining = ntds--; std->dma_addr = dma_map_single(whc->wusbhc.dev, std->pl_virt, pl_len, DMA_TO_DEVICE); + if (dma_mapping_error(whc->wusbhc.dev, std->dma_addr)) + return -EFAULT; + std->ntds_remaining = ntds--; } } return 0; @@ -618,6 +620,8 @@ static int qset_add_urb_sg_linearize(struct whc *whc, struct whc_qset *qset, std->dma_addr = dma_map_single(&whc->umc->dev, std->bounce_buf, std->len, is_out ? DMA_TO_DEVICE : DMA_FROM_DEVICE); + if (dma_mapping_error(&whc->umc->dev, std->dma_addr)) + return -EFAULT; if (qset_fill_page_list(whc, std, mem_flags) < 0) return -ENOMEM; -- 1.9.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] USB: whci-hcd: add more checks for dma mapping error 2016-03-25 20:23 [PATCH] USB: whci-hcd: add more checks for dma mapping error Alexey Khoroshilov @ 2016-03-25 22:03 ` Vladimir Zapolskiy 2016-03-25 22:56 ` Alexey Khoroshilov 0 siblings, 1 reply; 6+ messages in thread From: Vladimir Zapolskiy @ 2016-03-25 22:03 UTC (permalink / raw) To: Alexey Khoroshilov, Greg Kroah-Hartman Cc: linux-usb, linux-kernel, ldv-project On 25.03.2016 22:23, Alexey Khoroshilov wrote: > Fixing checks for dma mapping error in qset_fill_page_list(), > I have missed two similar problems in qset_add_urb_sg() and > in qset_add_urb_sg_linearize(). > > Found by Linux Driver Verification project (linuxtesting.org). > > Signed-off-by: Alexey Khoroshilov <khoroshilov@ispras.ru> > --- > drivers/usb/host/whci/qset.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/drivers/usb/host/whci/qset.c b/drivers/usb/host/whci/qset.c > index 1a8e960d073b..a8e9b618e643 100644 > --- a/drivers/usb/host/whci/qset.c > +++ b/drivers/usb/host/whci/qset.c > @@ -535,9 +535,11 @@ static int qset_add_urb_sg(struct whc *whc, struct whc_qset *qset, struct urb *u > list_for_each_entry(std, &qset->stds, list_node) { > if (std->ntds_remaining == -1) { > pl_len = std->num_pointers * sizeof(struct whc_page_list_entry); > - std->ntds_remaining = ntds--; > std->dma_addr = dma_map_single(whc->wusbhc.dev, std->pl_virt, > pl_len, DMA_TO_DEVICE); > + if (dma_mapping_error(whc->wusbhc.dev, std->dma_addr)) > + return -EFAULT; Resources are leaked on error path: * std->pl_virt -- most probably, at least it is freed on error path above, * dma mappings done in a loop before met error, > + std->ntds_remaining = ntds--; > } > } > return 0; > @@ -618,6 +620,8 @@ static int qset_add_urb_sg_linearize(struct whc *whc, struct whc_qset *qset, > > std->dma_addr = dma_map_single(&whc->umc->dev, std->bounce_buf, std->len, > is_out ? DMA_TO_DEVICE : DMA_FROM_DEVICE); > + if (dma_mapping_error(&whc->umc->dev, std->dma_addr)) > + return -EFAULT; > > if (qset_fill_page_list(whc, std, mem_flags) < 0) > return -ENOMEM; > -- With best wishes, Vladimir ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] USB: whci-hcd: add more checks for dma mapping error 2016-03-25 22:03 ` Vladimir Zapolskiy @ 2016-03-25 22:56 ` Alexey Khoroshilov 2016-03-26 0:20 ` Vladimir Zapolskiy 0 siblings, 1 reply; 6+ messages in thread From: Alexey Khoroshilov @ 2016-03-25 22:56 UTC (permalink / raw) To: Vladimir Zapolskiy, Greg Kroah-Hartman Cc: linux-usb, linux-kernel, ldv-project On 26.03.2016 01:03, Vladimir Zapolskiy wrote: > On 25.03.2016 22:23, Alexey Khoroshilov wrote: >> Fixing checks for dma mapping error in qset_fill_page_list(), >> I have missed two similar problems in qset_add_urb_sg() and >> in qset_add_urb_sg_linearize(). >> >> Found by Linux Driver Verification project (linuxtesting.org). >> >> Signed-off-by: Alexey Khoroshilov <khoroshilov@ispras.ru> >> --- >> drivers/usb/host/whci/qset.c | 6 +++++- >> 1 file changed, 5 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/usb/host/whci/qset.c b/drivers/usb/host/whci/qset.c >> index 1a8e960d073b..a8e9b618e643 100644 >> --- a/drivers/usb/host/whci/qset.c >> +++ b/drivers/usb/host/whci/qset.c >> @@ -535,9 +535,11 @@ static int qset_add_urb_sg(struct whc *whc, struct whc_qset *qset, struct urb *u >> list_for_each_entry(std, &qset->stds, list_node) { >> if (std->ntds_remaining == -1) { >> pl_len = std->num_pointers * sizeof(struct whc_page_list_entry); >> - std->ntds_remaining = ntds--; >> std->dma_addr = dma_map_single(whc->wusbhc.dev, std->pl_virt, >> pl_len, DMA_TO_DEVICE); >> + if (dma_mapping_error(whc->wusbhc.dev, std->dma_addr)) >> + return -EFAULT; > > Resources are leaked on error path: > * std->pl_virt -- most probably, at least it is freed on error path above, > * dma mappings done in a loop before met error, > As far as I can see, it is not the case. If qset_add_urb_sg() returns error code, the caller (qset_add_urb()) invokes qset_free_stds() that performs all resource deallocations. As for the error path above, I consider it as a typical krealloc() pattern, since it does not frees memory allocated at previous iterations of the cycle. Thank you, Alexey ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] USB: whci-hcd: add more checks for dma mapping error 2016-03-25 22:56 ` Alexey Khoroshilov @ 2016-03-26 0:20 ` Vladimir Zapolskiy 2016-03-26 19:42 ` [PATCH v2] " Alexey Khoroshilov 0 siblings, 1 reply; 6+ messages in thread From: Vladimir Zapolskiy @ 2016-03-26 0:20 UTC (permalink / raw) To: Alexey Khoroshilov, Greg Kroah-Hartman Cc: linux-usb, linux-kernel, ldv-project On 26.03.2016 00:56, Alexey Khoroshilov wrote: > On 26.03.2016 01:03, Vladimir Zapolskiy wrote: >> On 25.03.2016 22:23, Alexey Khoroshilov wrote: >>> Fixing checks for dma mapping error in qset_fill_page_list(), >>> I have missed two similar problems in qset_add_urb_sg() and >>> in qset_add_urb_sg_linearize(). >>> >>> Found by Linux Driver Verification project (linuxtesting.org). >>> >>> Signed-off-by: Alexey Khoroshilov <khoroshilov@ispras.ru> >>> --- >>> drivers/usb/host/whci/qset.c | 6 +++++- >>> 1 file changed, 5 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/usb/host/whci/qset.c b/drivers/usb/host/whci/qset.c >>> index 1a8e960d073b..a8e9b618e643 100644 >>> --- a/drivers/usb/host/whci/qset.c >>> +++ b/drivers/usb/host/whci/qset.c >>> @@ -535,9 +535,11 @@ static int qset_add_urb_sg(struct whc *whc, struct whc_qset *qset, struct urb *u >>> list_for_each_entry(std, &qset->stds, list_node) { >>> if (std->ntds_remaining == -1) { >>> pl_len = std->num_pointers * sizeof(struct whc_page_list_entry); >>> - std->ntds_remaining = ntds--; >>> std->dma_addr = dma_map_single(whc->wusbhc.dev, std->pl_virt, >>> pl_len, DMA_TO_DEVICE); >>> + if (dma_mapping_error(whc->wusbhc.dev, std->dma_addr)) >>> + return -EFAULT; >> >> Resources are leaked on error path: >> * std->pl_virt -- most probably, at least it is freed on error path above, >> * dma mappings done in a loop before met error, >> > > As far as I can see, it is not the case. > If qset_add_urb_sg() returns error code, the caller (qset_add_urb()) > invokes qset_free_stds() that performs all resource deallocations. Ok, but qset_free_std() lacks dma_mapping_error() check for mappings, will it try to unmap a nonexistent/invalid mapping? > As for the error path above, I consider it as a typical krealloc() > pattern, since it does not frees memory allocated at previous iterations > of the cycle. > The dynamically (re-)allocated memory is freed by qset_free_std(), that said kfree() and pointer assignment to NULL in qset_add_urb_sg() error path may be removed IMHO. -- With best wishes, Vladimir ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2] USB: whci-hcd: add more checks for dma mapping error 2016-03-26 0:20 ` Vladimir Zapolskiy @ 2016-03-26 19:42 ` Alexey Khoroshilov 2016-03-30 7:48 ` Vladimir Zapolskiy 0 siblings, 1 reply; 6+ messages in thread From: Alexey Khoroshilov @ 2016-03-26 19:42 UTC (permalink / raw) To: Greg Kroah-Hartman, Vladimir Zapolskiy Cc: Alexey Khoroshilov, linux-usb, linux-kernel, ldv-project Fixing checks for dma mapping error in qset_fill_page_list(), I have missed two similar problems in qset_add_urb_sg() and in qset_add_urb_sg_linearize(). v2: check validity of dma_addr with dma_mapping_error() in qset_free_std() as suggested by Vladimir Zapolskiy. Found by Linux Driver Verification project (linuxtesting.org). Signed-off-by: Alexey Khoroshilov <khoroshilov@ispras.ru> --- drivers/usb/host/whci/qset.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/drivers/usb/host/whci/qset.c b/drivers/usb/host/whci/qset.c index 1a8e960d073b..c0e6812426b3 100644 --- a/drivers/usb/host/whci/qset.c +++ b/drivers/usb/host/whci/qset.c @@ -314,7 +314,7 @@ void qset_free_std(struct whc *whc, struct whc_std *std) kfree(std->bounce_buf); } if (std->pl_virt) { - if (std->dma_addr) + if (!dma_mapping_error(whc->wusbhc.dev, std->dma_addr)) dma_unmap_single(whc->wusbhc.dev, std->dma_addr, std->num_pointers * sizeof(struct whc_page_list_entry), DMA_TO_DEVICE); @@ -535,9 +535,11 @@ static int qset_add_urb_sg(struct whc *whc, struct whc_qset *qset, struct urb *u list_for_each_entry(std, &qset->stds, list_node) { if (std->ntds_remaining == -1) { pl_len = std->num_pointers * sizeof(struct whc_page_list_entry); - std->ntds_remaining = ntds--; std->dma_addr = dma_map_single(whc->wusbhc.dev, std->pl_virt, pl_len, DMA_TO_DEVICE); + if (dma_mapping_error(whc->wusbhc.dev, std->dma_addr)) + return -EFAULT; + std->ntds_remaining = ntds--; } } return 0; @@ -618,6 +620,8 @@ static int qset_add_urb_sg_linearize(struct whc *whc, struct whc_qset *qset, std->dma_addr = dma_map_single(&whc->umc->dev, std->bounce_buf, std->len, is_out ? DMA_TO_DEVICE : DMA_FROM_DEVICE); + if (dma_mapping_error(&whc->umc->dev, std->dma_addr)) + return -EFAULT; if (qset_fill_page_list(whc, std, mem_flags) < 0) return -ENOMEM; -- 1.9.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2] USB: whci-hcd: add more checks for dma mapping error 2016-03-26 19:42 ` [PATCH v2] " Alexey Khoroshilov @ 2016-03-30 7:48 ` Vladimir Zapolskiy 0 siblings, 0 replies; 6+ messages in thread From: Vladimir Zapolskiy @ 2016-03-30 7:48 UTC (permalink / raw) To: Alexey Khoroshilov, Greg Kroah-Hartman Cc: linux-usb, linux-kernel, ldv-project Hi Alexey, On 26.03.2016 21:42, Alexey Khoroshilov wrote: > Fixing checks for dma mapping error in qset_fill_page_list(), > I have missed two similar problems in qset_add_urb_sg() and > in qset_add_urb_sg_linearize(). > > v2: check validity of dma_addr with dma_mapping_error() > in qset_free_std() as suggested by Vladimir Zapolskiy. > > Found by Linux Driver Verification project (linuxtesting.org). > > Signed-off-by: Alexey Khoroshilov <khoroshilov@ispras.ru> Reviewed-by: Vladimir Zapolskiy <vz@mleia.com> > --- > drivers/usb/host/whci/qset.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/drivers/usb/host/whci/qset.c b/drivers/usb/host/whci/qset.c > index 1a8e960d073b..c0e6812426b3 100644 > --- a/drivers/usb/host/whci/qset.c > +++ b/drivers/usb/host/whci/qset.c > @@ -314,7 +314,7 @@ void qset_free_std(struct whc *whc, struct whc_std *std) > kfree(std->bounce_buf); > } > if (std->pl_virt) { > - if (std->dma_addr) > + if (!dma_mapping_error(whc->wusbhc.dev, std->dma_addr)) > dma_unmap_single(whc->wusbhc.dev, std->dma_addr, > std->num_pointers * sizeof(struct whc_page_list_entry), > DMA_TO_DEVICE); > @@ -535,9 +535,11 @@ static int qset_add_urb_sg(struct whc *whc, struct whc_qset *qset, struct urb *u > list_for_each_entry(std, &qset->stds, list_node) { > if (std->ntds_remaining == -1) { > pl_len = std->num_pointers * sizeof(struct whc_page_list_entry); > - std->ntds_remaining = ntds--; > std->dma_addr = dma_map_single(whc->wusbhc.dev, std->pl_virt, > pl_len, DMA_TO_DEVICE); > + if (dma_mapping_error(whc->wusbhc.dev, std->dma_addr)) > + return -EFAULT; > + std->ntds_remaining = ntds--; > } > } > return 0; > @@ -618,6 +620,8 @@ static int qset_add_urb_sg_linearize(struct whc *whc, struct whc_qset *qset, > > std->dma_addr = dma_map_single(&whc->umc->dev, std->bounce_buf, std->len, > is_out ? DMA_TO_DEVICE : DMA_FROM_DEVICE); > + if (dma_mapping_error(&whc->umc->dev, std->dma_addr)) > + return -EFAULT; >From whc_probe() looks like &whc->umc->dev is the same as whc->wusbhc.dev, so the change is correct, but I would suggest to unify the pointer to a device. Still the driver has many problems, e.g. double kfree() -- error path in qset_fill_page_list() and qset_free_stds() etc. > if (qset_fill_page_list(whc, std, mem_flags) < 0) > return -ENOMEM; > -- With best wishes, Vladimir ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-03-30 7:48 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-03-25 20:23 [PATCH] USB: whci-hcd: add more checks for dma mapping error Alexey Khoroshilov 2016-03-25 22:03 ` Vladimir Zapolskiy 2016-03-25 22:56 ` Alexey Khoroshilov 2016-03-26 0:20 ` Vladimir Zapolskiy 2016-03-26 19:42 ` [PATCH v2] " Alexey Khoroshilov 2016-03-30 7:48 ` Vladimir Zapolskiy
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.