* [PATCH] swiotlb: add overflow checks to swiotlb_bounce
@ 2021-07-07 5:12 ` Dominique Martinet
0 siblings, 0 replies; 6+ messages in thread
From: Dominique Martinet @ 2021-07-07 5:12 UTC (permalink / raw)
To: Konrad Rzeszutek Wilk
Cc: Dominique Martinet, linux-kernel, Bumyong Lee, iommu,
Chanho Park, Robin Murphy, Christoph Hellwig
This is a follow-up on 5f89468e2f06 ("swiotlb: manipulate orig_addr
when tlb_addr has offset") which fixed unaligned dma mappings,
making sure the following overflows are caught:
- offset of the start of the slot within the device bigger than
requested address' offset, in other words if the base address
given in swiotlb_tbl_map_single to create the mapping (orig_addr)
was after the requested address for the sync (tlb_offset) in the
same block:
|------------------------------------------| block
<----------------------------> mapped part of the block
^
orig_addr
^
invalid tlb_addr for sync
- if the resulting offset was bigger than the allocation size
this one could happen if the mapping was not until the end. e.g.
|------------------------------------------| block
<---------------------> mapped part of the block
^ ^
orig_addr invalid tlb_addr
Both should never happen so print a warning and bail out without trying
to adjust the sizes/offsets: the first one could try to sync from
orig_addr to whatever is left of the requested size, but the later
really has nothing to sync there...
Signed-off-by: Dominique Martinet <dominique.martinet@atmark-techno.com>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Bumyong Lee <bumyong.lee@samsung.com>
Cc: Chanho Park <chanho61.park@samsung.com>
Cc: Christoph Hellwig <hch@lst.de>
---
Hi Konrad,
here's the follow up for the swiotlb/caamjr regression I had promissed.
It doesn't really change anything, and I confirmed I don't hit either of
the warnings on our board, but it's probably best to have as either
could really happen.
kernel/dma/swiotlb.c | 20 +++++++++++++++++---
1 file changed, 17 insertions(+), 3 deletions(-)
diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c
index e50df8d8f87e..23f8d0b168c5 100644
--- a/kernel/dma/swiotlb.c
+++ b/kernel/dma/swiotlb.c
@@ -354,13 +354,27 @@ static void swiotlb_bounce(struct device *dev, phys_addr_t tlb_addr, size_t size
size_t alloc_size = mem->slots[index].alloc_size;
unsigned long pfn = PFN_DOWN(orig_addr);
unsigned char *vaddr = phys_to_virt(tlb_addr);
- unsigned int tlb_offset;
+ unsigned int tlb_offset, orig_addr_offset;
if (orig_addr == INVALID_PHYS_ADDR)
return;
- tlb_offset = (tlb_addr & (IO_TLB_SIZE - 1)) -
- swiotlb_align_offset(dev, orig_addr);
+ tlb_offset = tlb_addr & (IO_TLB_SIZE - 1);
+ orig_addr_offset = swiotlb_align_offset(dev, orig_addr);
+ if (tlb_offset < orig_addr_offset) {
+ dev_WARN_ONCE(dev, 1,
+ "Access before mapping start detected. orig offset %u, requested offset %u.\n",
+ orig_addr_offset, tlb_offset);
+ return;
+ }
+
+ tlb_offset -= orig_addr_offset;
+ if (tlb_offset > alloc_size) {
+ dev_WARN_ONCE(dev, 1,
+ "Buffer overflow detected. Allocation size: %zu. Mapping size: %zu+%u.\n",
+ alloc_size, size, tlb_offset);
+ return;
+ }
orig_addr += tlb_offset;
alloc_size -= tlb_offset;
--
2.30.2
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
^ permalink raw reply related [flat|nested] 6+ messages in thread* RE: [PATCH] swiotlb: add overflow checks to swiotlb_bounce
2021-07-07 5:12 ` Dominique Martinet
@ 2021-07-08 0:59 ` 이범용
-1 siblings, 0 replies; 6+ messages in thread
From: 이범용 @ 2021-07-08 0:59 UTC (permalink / raw)
To: 'Dominique Martinet', 'Konrad Rzeszutek Wilk'
Cc: 'Marek Szyprowski', 'Robin Murphy',
iommu, linux-kernel, 'Chanho Park',
'Christoph Hellwig'
> This is a follow-up on 5f89468e2f06 ("swiotlb: manipulate orig_addr when
> tlb_addr has offset") which fixed unaligned dma mappings, making sure the
> following overflows are caught:
>
> - offset of the start of the slot within the device bigger than requested
> address' offset, in other words if the base address given in
> swiotlb_tbl_map_single to create the mapping (orig_addr) was after the
> requested address for the sync (tlb_offset) in the same block:
>
> |------------------------------------------| block
> <----------------------------> mapped part of the block
> ^
> orig_addr
> ^
> invalid tlb_addr for sync
>
> - if the resulting offset was bigger than the allocation size this one
> could happen if the mapping was not until the end. e.g.
>
> |------------------------------------------| block
> <---------------------> mapped part of the block
> ^ ^
> orig_addr invalid tlb_addr
>
> Both should never happen so print a warning and bail out without trying to
> adjust the sizes/offsets: the first one could try to sync from orig_addr
> to whatever is left of the requested size, but the later really has
> nothing to sync there...
>
> Signed-off-by: Dominique Martinet <dominique.martinet@atmark-techno.com>
> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Cc: Bumyong Lee <bumyong.lee@samsung.com>
Reviewed-by: Bumyong Lee <bumyong.lee@samsung.com
> Cc: Chanho Park <chanho61.park@samsung.com>
> Cc: Christoph Hellwig <hch@lst.de>
> ---
>
> Hi Konrad,
>
> here's the follow up for the swiotlb/caamjr regression I had promissed.
> It doesn't really change anything, and I confirmed I don't hit either of
> the warnings on our board, but it's probably best to have as either could
> really happen.
>
>
> kernel/dma/swiotlb.c | 20 +++++++++++++++++---
> 1 file changed, 17 insertions(+), 3 deletions(-)
>
> diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c index
> e50df8d8f87e..23f8d0b168c5 100644
> --- a/kernel/dma/swiotlb.c
> +++ b/kernel/dma/swiotlb.c
> @@ -354,13 +354,27 @@ static void swiotlb_bounce(struct device *dev,
> phys_addr_t tlb_addr, size_t size
> size_t alloc_size = mem->slots[index].alloc_size;
> unsigned long pfn = PFN_DOWN(orig_addr);
> unsigned char *vaddr = phys_to_virt(tlb_addr);
> - unsigned int tlb_offset;
> + unsigned int tlb_offset, orig_addr_offset;
>
> if (orig_addr == INVALID_PHYS_ADDR)
> return;
>
> - tlb_offset = (tlb_addr & (IO_TLB_SIZE - 1)) -
> - swiotlb_align_offset(dev, orig_addr);
> + tlb_offset = tlb_addr & (IO_TLB_SIZE - 1);
> + orig_addr_offset = swiotlb_align_offset(dev, orig_addr);
> + if (tlb_offset < orig_addr_offset) {
> + dev_WARN_ONCE(dev, 1,
> + "Access before mapping start detected. orig offset
%u,
> requested offset %u.\n",
> + orig_addr_offset, tlb_offset);
> + return;
> + }
> +
> + tlb_offset -= orig_addr_offset;
> + if (tlb_offset > alloc_size) {
> + dev_WARN_ONCE(dev, 1,
> + "Buffer overflow detected. Allocation size: %zu.
> Mapping size: %zu+%u.\n",
> + alloc_size, size, tlb_offset);
> + return;
> + }
>
> orig_addr += tlb_offset;
> alloc_size -= tlb_offset;
> --
> 2.30.2
^ permalink raw reply [flat|nested] 6+ messages in thread* RE: [PATCH] swiotlb: add overflow checks to swiotlb_bounce
@ 2021-07-08 0:59 ` 이범용
0 siblings, 0 replies; 6+ messages in thread
From: 이범용 @ 2021-07-08 0:59 UTC (permalink / raw)
To: 'Dominique Martinet', 'Konrad Rzeszutek Wilk'
Cc: linux-kernel, iommu, 'Chanho Park',
'Robin Murphy', 'Christoph Hellwig'
> This is a follow-up on 5f89468e2f06 ("swiotlb: manipulate orig_addr when
> tlb_addr has offset") which fixed unaligned dma mappings, making sure the
> following overflows are caught:
>
> - offset of the start of the slot within the device bigger than requested
> address' offset, in other words if the base address given in
> swiotlb_tbl_map_single to create the mapping (orig_addr) was after the
> requested address for the sync (tlb_offset) in the same block:
>
> |------------------------------------------| block
> <----------------------------> mapped part of the block
> ^
> orig_addr
> ^
> invalid tlb_addr for sync
>
> - if the resulting offset was bigger than the allocation size this one
> could happen if the mapping was not until the end. e.g.
>
> |------------------------------------------| block
> <---------------------> mapped part of the block
> ^ ^
> orig_addr invalid tlb_addr
>
> Both should never happen so print a warning and bail out without trying to
> adjust the sizes/offsets: the first one could try to sync from orig_addr
> to whatever is left of the requested size, but the later really has
> nothing to sync there...
>
> Signed-off-by: Dominique Martinet <dominique.martinet@atmark-techno.com>
> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Cc: Bumyong Lee <bumyong.lee@samsung.com>
Reviewed-by: Bumyong Lee <bumyong.lee@samsung.com
> Cc: Chanho Park <chanho61.park@samsung.com>
> Cc: Christoph Hellwig <hch@lst.de>
> ---
>
> Hi Konrad,
>
> here's the follow up for the swiotlb/caamjr regression I had promissed.
> It doesn't really change anything, and I confirmed I don't hit either of
> the warnings on our board, but it's probably best to have as either could
> really happen.
>
>
> kernel/dma/swiotlb.c | 20 +++++++++++++++++---
> 1 file changed, 17 insertions(+), 3 deletions(-)
>
> diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c index
> e50df8d8f87e..23f8d0b168c5 100644
> --- a/kernel/dma/swiotlb.c
> +++ b/kernel/dma/swiotlb.c
> @@ -354,13 +354,27 @@ static void swiotlb_bounce(struct device *dev,
> phys_addr_t tlb_addr, size_t size
> size_t alloc_size = mem->slots[index].alloc_size;
> unsigned long pfn = PFN_DOWN(orig_addr);
> unsigned char *vaddr = phys_to_virt(tlb_addr);
> - unsigned int tlb_offset;
> + unsigned int tlb_offset, orig_addr_offset;
>
> if (orig_addr == INVALID_PHYS_ADDR)
> return;
>
> - tlb_offset = (tlb_addr & (IO_TLB_SIZE - 1)) -
> - swiotlb_align_offset(dev, orig_addr);
> + tlb_offset = tlb_addr & (IO_TLB_SIZE - 1);
> + orig_addr_offset = swiotlb_align_offset(dev, orig_addr);
> + if (tlb_offset < orig_addr_offset) {
> + dev_WARN_ONCE(dev, 1,
> + "Access before mapping start detected. orig offset
%u,
> requested offset %u.\n",
> + orig_addr_offset, tlb_offset);
> + return;
> + }
> +
> + tlb_offset -= orig_addr_offset;
> + if (tlb_offset > alloc_size) {
> + dev_WARN_ONCE(dev, 1,
> + "Buffer overflow detected. Allocation size: %zu.
> Mapping size: %zu+%u.\n",
> + alloc_size, size, tlb_offset);
> + return;
> + }
>
> orig_addr += tlb_offset;
> alloc_size -= tlb_offset;
> --
> 2.30.2
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] swiotlb: add overflow checks to swiotlb_bounce
2021-07-07 5:12 ` Dominique Martinet
@ 2021-07-13 23:54 ` Konrad Rzeszutek Wilk
-1 siblings, 0 replies; 6+ messages in thread
From: Konrad Rzeszutek Wilk @ 2021-07-13 23:54 UTC (permalink / raw)
To: Dominique Martinet
Cc: Konrad Rzeszutek Wilk, linux-kernel, Bumyong Lee, iommu,
Chanho Park, Robin Murphy, Christoph Hellwig
On Wed, Jul 07, 2021 at 02:12:54PM +0900, Dominique Martinet wrote:
> This is a follow-up on 5f89468e2f06 ("swiotlb: manipulate orig_addr
> when tlb_addr has offset") which fixed unaligned dma mappings,
> making sure the following overflows are caught:
>
> - offset of the start of the slot within the device bigger than
> requested address' offset, in other words if the base address
> given in swiotlb_tbl_map_single to create the mapping (orig_addr)
> was after the requested address for the sync (tlb_offset) in the
> same block:
>
> |------------------------------------------| block
> <----------------------------> mapped part of the block
> ^
> orig_addr
> ^
> invalid tlb_addr for sync
>
> - if the resulting offset was bigger than the allocation size
> this one could happen if the mapping was not until the end. e.g.
>
> |------------------------------------------| block
> <---------------------> mapped part of the block
> ^ ^
> orig_addr invalid tlb_addr
>
> Both should never happen so print a warning and bail out without trying
> to adjust the sizes/offsets: the first one could try to sync from
> orig_addr to whatever is left of the requested size, but the later
> really has nothing to sync there...
>
> Signed-off-by: Dominique Martinet <dominique.martinet@atmark-techno.com>
> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Cc: Bumyong Lee <bumyong.lee@samsung.com>
> Cc: Chanho Park <chanho61.park@samsung.com>
> Cc: Christoph Hellwig <hch@lst.de>
> ---
>
> Hi Konrad,
>
> here's the follow up for the swiotlb/caamjr regression I had promissed.
Awesome!
> It doesn't really change anything, and I confirmed I don't hit either of
> the warnings on our board, but it's probably best to have as either
> could really happen.
:nods:
I put it in the devel/for-linus-5.14 and linux-next. Thank you!
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] swiotlb: add overflow checks to swiotlb_bounce
@ 2021-07-13 23:54 ` Konrad Rzeszutek Wilk
0 siblings, 0 replies; 6+ messages in thread
From: Konrad Rzeszutek Wilk @ 2021-07-13 23:54 UTC (permalink / raw)
To: Dominique Martinet
Cc: Konrad Rzeszutek Wilk, linux-kernel, Bumyong Lee, iommu,
Chanho Park, Robin Murphy, Christoph Hellwig
On Wed, Jul 07, 2021 at 02:12:54PM +0900, Dominique Martinet wrote:
> This is a follow-up on 5f89468e2f06 ("swiotlb: manipulate orig_addr
> when tlb_addr has offset") which fixed unaligned dma mappings,
> making sure the following overflows are caught:
>
> - offset of the start of the slot within the device bigger than
> requested address' offset, in other words if the base address
> given in swiotlb_tbl_map_single to create the mapping (orig_addr)
> was after the requested address for the sync (tlb_offset) in the
> same block:
>
> |------------------------------------------| block
> <----------------------------> mapped part of the block
> ^
> orig_addr
> ^
> invalid tlb_addr for sync
>
> - if the resulting offset was bigger than the allocation size
> this one could happen if the mapping was not until the end. e.g.
>
> |------------------------------------------| block
> <---------------------> mapped part of the block
> ^ ^
> orig_addr invalid tlb_addr
>
> Both should never happen so print a warning and bail out without trying
> to adjust the sizes/offsets: the first one could try to sync from
> orig_addr to whatever is left of the requested size, but the later
> really has nothing to sync there...
>
> Signed-off-by: Dominique Martinet <dominique.martinet@atmark-techno.com>
> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Cc: Bumyong Lee <bumyong.lee@samsung.com>
> Cc: Chanho Park <chanho61.park@samsung.com>
> Cc: Christoph Hellwig <hch@lst.de>
> ---
>
> Hi Konrad,
>
> here's the follow up for the swiotlb/caamjr regression I had promissed.
Awesome!
> It doesn't really change anything, and I confirmed I don't hit either of
> the warnings on our board, but it's probably best to have as either
> could really happen.
:nods:
I put it in the devel/for-linus-5.14 and linux-next. Thank you!
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
^ permalink raw reply [flat|nested] 6+ messages in thread