* [PATCH] iommu/dma: Fix calculation overflow in __finalise_sg() @ 2019-06-22 4:38 Nicolin Chen 2019-07-01 12:21 ` Joerg Roedel 0 siblings, 1 reply; 7+ messages in thread From: Nicolin Chen @ 2019-06-22 4:38 UTC (permalink / raw) To: joro; +Cc: robin.murphy, iommu, linux-kernel The max_len is a u32 type variable so the calculation on the left hand of the last if-condition will potentially overflow when a cur_len gets closer to UINT_MAX -- note that there're drivers setting max_seg_size to UINT_MAX: drivers/dma/dw-edma/dw-edma-core.c:745: dma_set_max_seg_size(dma->dev, U32_MAX); drivers/dma/dma-axi-dmac.c:871: dma_set_max_seg_size(&pdev->dev, UINT_MAX); drivers/mmc/host/renesas_sdhi_internal_dmac.c:338: dma_set_max_seg_size(dev, 0xffffffff); drivers/nvme/host/pci.c:2520: dma_set_max_seg_size(dev->dev, 0xffffffff); So this patch just casts the cur_len in the calculation to a size_t type to fix the overflow issue, as it's not necessary to change the type of cur_len after all. Fixes: 809eac54cdd6 ("iommu/dma: Implement scatterlist segment merging") Cc: stable@vger.kernel.org Signed-off-by: Nicolin Chen <nicoleotsuka@gmail.com> --- drivers/iommu/dma-iommu.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c index a9f13313a22f..676b7ecd451e 100644 --- a/drivers/iommu/dma-iommu.c +++ b/drivers/iommu/dma-iommu.c @@ -764,7 +764,7 @@ static int __finalise_sg(struct device *dev, struct scatterlist *sg, int nents, * - and wouldn't make the resulting output segment too long */ if (cur_len && !s_iova_off && (dma_addr & seg_mask) && - (cur_len + s_length <= max_len)) { + ((size_t)cur_len + s_length <= max_len)) { /* ...then concatenate it with the previous one */ cur_len += s_length; } else { -- 2.17.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] iommu/dma: Fix calculation overflow in __finalise_sg() 2019-06-22 4:38 [PATCH] iommu/dma: Fix calculation overflow in __finalise_sg() Nicolin Chen @ 2019-07-01 12:21 ` Joerg Roedel 2019-07-01 12:39 ` Robin Murphy 0 siblings, 1 reply; 7+ messages in thread From: Joerg Roedel @ 2019-07-01 12:21 UTC (permalink / raw) To: Nicolin Chen; +Cc: robin.murphy, iommu, linux-kernel On Fri, Jun 21, 2019 at 09:38:14PM -0700, Nicolin Chen wrote: > The max_len is a u32 type variable so the calculation on the > left hand of the last if-condition will potentially overflow > when a cur_len gets closer to UINT_MAX -- note that there're > drivers setting max_seg_size to UINT_MAX: > drivers/dma/dw-edma/dw-edma-core.c:745: > dma_set_max_seg_size(dma->dev, U32_MAX); > drivers/dma/dma-axi-dmac.c:871: > dma_set_max_seg_size(&pdev->dev, UINT_MAX); > drivers/mmc/host/renesas_sdhi_internal_dmac.c:338: > dma_set_max_seg_size(dev, 0xffffffff); > drivers/nvme/host/pci.c:2520: > dma_set_max_seg_size(dev->dev, 0xffffffff); > > So this patch just casts the cur_len in the calculation to a > size_t type to fix the overflow issue, as it's not necessary > to change the type of cur_len after all. > > Fixes: 809eac54cdd6 ("iommu/dma: Implement scatterlist segment merging") > Cc: stable@vger.kernel.org > Signed-off-by: Nicolin Chen <nicoleotsuka@gmail.com> Looks good to me, but I let Robin take a look too before I apply it, Robin? ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] iommu/dma: Fix calculation overflow in __finalise_sg() 2019-07-01 12:21 ` Joerg Roedel @ 2019-07-01 12:39 ` Robin Murphy 2019-07-01 21:50 ` Nicolin Chen 0 siblings, 1 reply; 7+ messages in thread From: Robin Murphy @ 2019-07-01 12:39 UTC (permalink / raw) To: Joerg Roedel, Nicolin Chen; +Cc: iommu, linux-kernel On 01/07/2019 13:21, Joerg Roedel wrote: > On Fri, Jun 21, 2019 at 09:38:14PM -0700, Nicolin Chen wrote: >> The max_len is a u32 type variable so the calculation on the >> left hand of the last if-condition will potentially overflow >> when a cur_len gets closer to UINT_MAX -- note that there're >> drivers setting max_seg_size to UINT_MAX: >> drivers/dma/dw-edma/dw-edma-core.c:745: >> dma_set_max_seg_size(dma->dev, U32_MAX); >> drivers/dma/dma-axi-dmac.c:871: >> dma_set_max_seg_size(&pdev->dev, UINT_MAX); >> drivers/mmc/host/renesas_sdhi_internal_dmac.c:338: >> dma_set_max_seg_size(dev, 0xffffffff); >> drivers/nvme/host/pci.c:2520: >> dma_set_max_seg_size(dev->dev, 0xffffffff); >> >> So this patch just casts the cur_len in the calculation to a >> size_t type to fix the overflow issue, as it's not necessary >> to change the type of cur_len after all. >> >> Fixes: 809eac54cdd6 ("iommu/dma: Implement scatterlist segment merging") >> Cc: stable@vger.kernel.org >> Signed-off-by: Nicolin Chen <nicoleotsuka@gmail.com> > > Looks good to me, but I let Robin take a look too before I apply it, > Robin? I'll need to take a closer look at how exactly an overflow would happen here (just got back off some holiday), but my immediate thought is that if this is a real problem, then what about 32-bit builds where size_t would still overflow? Robin. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] iommu/dma: Fix calculation overflow in __finalise_sg() 2019-07-01 12:39 ` Robin Murphy @ 2019-07-01 21:50 ` Nicolin Chen 2019-07-02 10:40 ` Robin Murphy 0 siblings, 1 reply; 7+ messages in thread From: Nicolin Chen @ 2019-07-01 21:50 UTC (permalink / raw) To: Robin Murphy; +Cc: Joerg Roedel, iommu, linux-kernel Hi Robin, On Mon, Jul 01, 2019 at 01:39:55PM +0100, Robin Murphy wrote: > > > The max_len is a u32 type variable so the calculation on the > > > left hand of the last if-condition will potentially overflow > > > when a cur_len gets closer to UINT_MAX -- note that there're > > > drivers setting max_seg_size to UINT_MAX: > > > drivers/dma/dw-edma/dw-edma-core.c:745: > > > dma_set_max_seg_size(dma->dev, U32_MAX); > > > drivers/dma/dma-axi-dmac.c:871: > > > dma_set_max_seg_size(&pdev->dev, UINT_MAX); > > > drivers/mmc/host/renesas_sdhi_internal_dmac.c:338: > > > dma_set_max_seg_size(dev, 0xffffffff); > > > drivers/nvme/host/pci.c:2520: > > > dma_set_max_seg_size(dev->dev, 0xffffffff); > > > > > > So this patch just casts the cur_len in the calculation to a > > > size_t type to fix the overflow issue, as it's not necessary > > > to change the type of cur_len after all. > > > > > > Fixes: 809eac54cdd6 ("iommu/dma: Implement scatterlist segment merging") > > > Cc: stable@vger.kernel.org > > > Signed-off-by: Nicolin Chen <nicoleotsuka@gmail.com> > > > > Looks good to me, but I let Robin take a look too before I apply it, > > Robin? > I'll need to take a closer look at how exactly an overflow would happen here It was triggered by a test case that was trying to map a 4GB dma_buf (1000+ nents in the scatterlist). This function then seemed to reduce the nents by merging most of them, probably because they were contiguous. > (just got back off some holiday), but my immediate thought is that if this > is a real problem, then what about 32-bit builds where size_t would still > overflow? I think most of callers are also using size_t type for their size parameters like dma_buf, so the cur_len + s_length will unlikely go higher than UINT_MAX. But just in case that some driver allocates a large sg with a size parameter defined in 64-bit and uses this map() function, so it might be safer to change to "size_t" here to "u64"? Thank you Nicolin ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] iommu/dma: Fix calculation overflow in __finalise_sg() 2019-07-01 21:50 ` Nicolin Chen @ 2019-07-02 10:40 ` Robin Murphy 2019-07-02 21:04 ` Nicolin Chen 0 siblings, 1 reply; 7+ messages in thread From: Robin Murphy @ 2019-07-02 10:40 UTC (permalink / raw) To: Nicolin Chen; +Cc: Joerg Roedel, iommu, linux-kernel On 01/07/2019 22:50, Nicolin Chen wrote: > Hi Robin, > > On Mon, Jul 01, 2019 at 01:39:55PM +0100, Robin Murphy wrote: >>>> The max_len is a u32 type variable so the calculation on the >>>> left hand of the last if-condition will potentially overflow >>>> when a cur_len gets closer to UINT_MAX -- note that there're >>>> drivers setting max_seg_size to UINT_MAX: >>>> drivers/dma/dw-edma/dw-edma-core.c:745: >>>> dma_set_max_seg_size(dma->dev, U32_MAX); >>>> drivers/dma/dma-axi-dmac.c:871: >>>> dma_set_max_seg_size(&pdev->dev, UINT_MAX); >>>> drivers/mmc/host/renesas_sdhi_internal_dmac.c:338: >>>> dma_set_max_seg_size(dev, 0xffffffff); >>>> drivers/nvme/host/pci.c:2520: >>>> dma_set_max_seg_size(dev->dev, 0xffffffff); >>>> >>>> So this patch just casts the cur_len in the calculation to a >>>> size_t type to fix the overflow issue, as it's not necessary >>>> to change the type of cur_len after all. >>>> >>>> Fixes: 809eac54cdd6 ("iommu/dma: Implement scatterlist segment merging") >>>> Cc: stable@vger.kernel.org >>>> Signed-off-by: Nicolin Chen <nicoleotsuka@gmail.com> >>> >>> Looks good to me, but I let Robin take a look too before I apply it, >>> Robin? >> I'll need to take a closer look at how exactly an overflow would happen here > > It was triggered by a test case that was trying to map a 4GB > dma_buf (1000+ nents in the scatterlist). This function then > seemed to reduce the nents by merging most of them, probably > because they were contiguous. So, looking at the code now, the actual problem seems to be that this overflow allows segments to be merged to or beyond 4GB total length, thus sg->dma_length for the resulting segment also ends up wrapped and truncated, and the caller will end up going downhill from there; is that correct? >> (just got back off some holiday), but my immediate thought is that if this >> is a real problem, then what about 32-bit builds where size_t would still >> overflow? > > I think most of callers are also using size_t type for their > size parameters like dma_buf, so the cur_len + s_length will > unlikely go higher than UINT_MAX. But just in case that some > driver allocates a large sg with a size parameter defined in > 64-bit and uses this map() function, so it might be safer to > change to "size_t" here to "u64"? On reflection, I don't really think that size_t fits here anyway, since all the members of the incoming struct scatterlist are unsigned int too. Does the patch below work? Robin. ----->8----- From: Robin Murphy <robin.murphy@arm.com> Subject: [PATCH] iommu/dma: Handle SG length overflow better Since scatterlist dimensions are all unsigned ints, in the relatively rare cases where a device's max_segment_size is set to UINT_MAX, then the "cur_len + s_length <= max_len" check in __finalise_sg() will always return true. As a result, the corner case of such a device mapping an excessively large scatterlist which is mergeable to or beyond a total length of 4GB can lead to overflow and a bogus truncated dma_length in the resulting segment. As we already assume that any single segment must be no longer than max_len to begin with, this can easily be addressed by reshuffling the comparison. Fixes: 809eac54cdd6 ("iommu/dma: Implement scatterlist segment merging") Reported-by: Nicolin Chen <nicoleotsuka@gmail.com> Signed-off-by: Robin Murphy <robin.murphy@arm.com> --- drivers/iommu/dma-iommu.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c index 129c4badf9ae..8de6cf623362 100644 --- a/drivers/iommu/dma-iommu.c +++ b/drivers/iommu/dma-iommu.c @@ -721,7 +721,7 @@ static int __finalise_sg(struct device *dev, struct scatterlist *sg, int nents, * - and wouldn't make the resulting output segment too long */ if (cur_len && !s_iova_off && (dma_addr & seg_mask) && - (cur_len + s_length <= max_len)) { + (max_len - cur_len >= s_length)) { /* ...then concatenate it with the previous one */ cur_len += s_length; } else { ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] iommu/dma: Fix calculation overflow in __finalise_sg() 2019-07-02 10:40 ` Robin Murphy @ 2019-07-02 21:04 ` Nicolin Chen 2019-07-25 17:36 ` Nicolin Chen 0 siblings, 1 reply; 7+ messages in thread From: Nicolin Chen @ 2019-07-02 21:04 UTC (permalink / raw) To: Robin Murphy; +Cc: Joerg Roedel, iommu, linux-kernel On Tue, Jul 02, 2019 at 11:40:02AM +0100, Robin Murphy wrote: > On reflection, I don't really think that size_t fits here anyway, since > all the members of the incoming struct scatterlist are unsigned int too. > Does the patch below work? Yes. > ----->8----- > From: Robin Murphy <robin.murphy@arm.com> > Subject: [PATCH] iommu/dma: Handle SG length overflow better > > Since scatterlist dimensions are all unsigned ints, in the relatively > rare cases where a device's max_segment_size is set to UINT_MAX, then > the "cur_len + s_length <= max_len" check in __finalise_sg() will always > return true. As a result, the corner case of such a device mapping an > excessively large scatterlist which is mergeable to or beyond a total > length of 4GB can lead to overflow and a bogus truncated dma_length in > the resulting segment. > > As we already assume that any single segment must be no longer than > max_len to begin with, this can easily be addressed by reshuffling the > comparison. > > Fixes: 809eac54cdd6 ("iommu/dma: Implement scatterlist segment merging") > Reported-by: Nicolin Chen <nicoleotsuka@gmail.com> > Signed-off-by: Robin Murphy <robin.murphy@arm.com> Tested-by: Nicolin Chen <nicoleotsuka@gmail.com> Thank you! > --- > drivers/iommu/dma-iommu.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c > index 129c4badf9ae..8de6cf623362 100644 > --- a/drivers/iommu/dma-iommu.c > +++ b/drivers/iommu/dma-iommu.c > @@ -721,7 +721,7 @@ static int __finalise_sg(struct device *dev, struct scatterlist *sg, int nents, > * - and wouldn't make the resulting output segment too long > */ > if (cur_len && !s_iova_off && (dma_addr & seg_mask) && > - (cur_len + s_length <= max_len)) { > + (max_len - cur_len >= s_length)) { > /* ...then concatenate it with the previous one */ > cur_len += s_length; > } else { ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] iommu/dma: Fix calculation overflow in __finalise_sg() 2019-07-02 21:04 ` Nicolin Chen @ 2019-07-25 17:36 ` Nicolin Chen 0 siblings, 0 replies; 7+ messages in thread From: Nicolin Chen @ 2019-07-25 17:36 UTC (permalink / raw) To: Robin Murphy; +Cc: Joerg Roedel, iommu, linux-kernel Sorry to ping this but it's been a while. Robin, did you get a chance to resend your version? Thanks Nicolin On Tue, Jul 02, 2019 at 02:04:01PM -0700, Nicolin Chen wrote: > On Tue, Jul 02, 2019 at 11:40:02AM +0100, Robin Murphy wrote: > > On reflection, I don't really think that size_t fits here anyway, since > > all the members of the incoming struct scatterlist are unsigned int too. > > Does the patch below work? > > Yes. > > > ----->8----- > > From: Robin Murphy <robin.murphy@arm.com> > > Subject: [PATCH] iommu/dma: Handle SG length overflow better > > > > Since scatterlist dimensions are all unsigned ints, in the relatively > > rare cases where a device's max_segment_size is set to UINT_MAX, then > > the "cur_len + s_length <= max_len" check in __finalise_sg() will always > > return true. As a result, the corner case of such a device mapping an > > excessively large scatterlist which is mergeable to or beyond a total > > length of 4GB can lead to overflow and a bogus truncated dma_length in > > the resulting segment. > > > > As we already assume that any single segment must be no longer than > > max_len to begin with, this can easily be addressed by reshuffling the > > comparison. > > > > Fixes: 809eac54cdd6 ("iommu/dma: Implement scatterlist segment merging") > > Reported-by: Nicolin Chen <nicoleotsuka@gmail.com> > > Signed-off-by: Robin Murphy <robin.murphy@arm.com> > > Tested-by: Nicolin Chen <nicoleotsuka@gmail.com> > > Thank you! > > > --- > > drivers/iommu/dma-iommu.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c > > index 129c4badf9ae..8de6cf623362 100644 > > --- a/drivers/iommu/dma-iommu.c > > +++ b/drivers/iommu/dma-iommu.c > > @@ -721,7 +721,7 @@ static int __finalise_sg(struct device *dev, struct scatterlist *sg, int nents, > > * - and wouldn't make the resulting output segment too long > > */ > > if (cur_len && !s_iova_off && (dma_addr & seg_mask) && > > - (cur_len + s_length <= max_len)) { > > + (max_len - cur_len >= s_length)) { > > /* ...then concatenate it with the previous one */ > > cur_len += s_length; > > } else { ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2019-07-25 17:35 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-06-22 4:38 [PATCH] iommu/dma: Fix calculation overflow in __finalise_sg() Nicolin Chen 2019-07-01 12:21 ` Joerg Roedel 2019-07-01 12:39 ` Robin Murphy 2019-07-01 21:50 ` Nicolin Chen 2019-07-02 10:40 ` Robin Murphy 2019-07-02 21:04 ` Nicolin Chen 2019-07-25 17:36 ` Nicolin Chen
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).