[PATCH v1 0/3] crypto: ccp - CCP driver updates 2015-05-26

[PATCH v1 0/3] crypto: ccp - CCP driver updates 2015-05-26

[Tom Lendacky]

The following patches are included in this driver update series:

- Remove the checking and setting of the device dma_mask field
- Remove an unused field from a structure to help avoid any confusion
- Protect against poorly marked end of scatter-gather list
 
This patch series is based on cryptodev-2.6.

---

Tom Lendacky (3):
      crypto: ccp - Remove manual check and set of dma_mask pointer
      crypto: ccp - Remove unused structure field
      crypto: ccp - Protect against poorly marked end of sg list


 drivers/crypto/ccp/ccp-ops.c      |   20 +++++++++++++++++---
 drivers/crypto/ccp/ccp-platform.c |    2 --
 2 files changed, 17 insertions(+), 5 deletions(-)

-- 
Tom Lendacky

[PATCH v1 1/3] crypto: ccp - Remove manual check and set of dma_mask pointer

[Tom Lendacky]

The underlying device support will set the device dma_mask pointer
if DMA is set up properly for the device.  Remove the check for and
assignment of dma_mask when it is null. Instead, just error out if
the dma_set_mask_and_coherent function fails because dma_mask is null.

Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
---
 drivers/crypto/ccp/ccp-platform.c |    2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/crypto/ccp/ccp-platform.c b/drivers/crypto/ccp/ccp-platform.c
index b1c20b2..c0aa5c5 100644
--- a/drivers/crypto/ccp/ccp-platform.c
+++ b/drivers/crypto/ccp/ccp-platform.c
@@ -174,8 +174,6 @@ static int ccp_platform_probe(struct platform_device *pdev)
 	}
 	ccp->io_regs = ccp->io_map;
 
-	if (!dev->dma_mask)
-		dev->dma_mask = &dev->coherent_dma_mask;
 	ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(48));
 	if (ret) {
 		dev_err(dev, "dma_set_mask_and_coherent failed (%d)\n", ret);

[PATCH v1 2/3] crypto: ccp - Remove unused structure field

[Tom Lendacky]

Remove the length field from the ccp_sg_workarea since it is unused.

Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
---
 drivers/crypto/ccp/ccp-ops.c |    2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/crypto/ccp/ccp-ops.c b/drivers/crypto/ccp/ccp-ops.c
index 71f2e3c..542453c 100644
--- a/drivers/crypto/ccp/ccp-ops.c
+++ b/drivers/crypto/ccp/ccp-ops.c
@@ -53,7 +53,6 @@ struct ccp_dm_workarea {
 struct ccp_sg_workarea {
 	struct scatterlist *sg;
 	unsigned int nents;
-	unsigned int length;
 
 	struct scatterlist *dma_sg;
 	struct device *dma_dev;
@@ -497,7 +496,6 @@ static int ccp_init_sg_workarea(struct ccp_sg_workarea *wa, struct device *dev,
 		return 0;
 
 	wa->nents = sg_nents(sg);
-	wa->length = sg->length;
 	wa->bytes_left = len;
 	wa->sg_used = 0;
 

[PATCH v1 3/3] crypto: ccp - Protect against poorly marked end of sg list

[Tom Lendacky]

Scatter gather lists can be created with more available entries than are
actually used (e.g. using sg_init_table() to reserve a specific number
of sg entries, but in actuality using something less than that based on
the data length).  The caller sometimes fails to mark the last entry
with sg_mark_end().  In these cases, sg_nents() will return the original
size of the sg list as opposed to the actual number of sg entries that
contain valid data.

On arm64, if the sg_nents() value is used in a call to dma_map_sg() in
this situation, then it causes a BUG_ON in lib/swiotlb.c because an
"empty" sg list entry results in dma_capable() returning false and
swiotlb trying to create a bounce buffer of size 0. This occurred in
the userspace crypto interface before being fixed by

0f477b655a52 ("crypto: algif - Mark sgl end at the end of data")

Protect against this in the future by counting the number of sg entries
needed to meet the length requirement and supplying that value to
dma_map_sg().

Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
---
 drivers/crypto/ccp/ccp-ops.c |   18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/drivers/crypto/ccp/ccp-ops.c b/drivers/crypto/ccp/ccp-ops.c
index 542453c..8377ed6 100644
--- a/drivers/crypto/ccp/ccp-ops.c
+++ b/drivers/crypto/ccp/ccp-ops.c
@@ -477,6 +477,22 @@ static u32 ccp_gen_jobid(struct ccp_device *ccp)
 	return atomic_inc_return(&ccp->current_id) & CCP_JOBID_MASK;
 }
 
+static int ccp_sg_nents(struct scatterlist *sg, u64 len)
+{
+	int nents = 0;
+
+	while (sg && len) {
+		nents++;
+		if (sg->length > len)
+			break;
+
+		len -= sg->length;
+		sg = sg_next(sg);
+	}
+
+	return nents;
+}
+
 static void ccp_sg_free(struct ccp_sg_workarea *wa)
 {
 	if (wa->dma_count)
@@ -495,7 +511,7 @@ static int ccp_init_sg_workarea(struct ccp_sg_workarea *wa, struct device *dev,
 	if (!sg)
 		return 0;
 
-	wa->nents = sg_nents(sg);
+	wa->nents = ccp_sg_nents(sg, len);
 	wa->bytes_left = len;
 	wa->sg_used = 0;
 

Re: [PATCH v1 3/3] crypto: ccp - Protect against poorly marked end of sg list

[Herbert Xu]

Tom Lendacky <thomas.lendacky@amd.com> wrote:
> Scatter gather lists can be created with more available entries than are
> actually used (e.g. using sg_init_table() to reserve a specific number
> of sg entries, but in actuality using something less than that based on
> the data length).  The caller sometimes fails to mark the last entry
> with sg_mark_end().  In these cases, sg_nents() will return the original
> size of the sg list as opposed to the actual number of sg entries that
> contain valid data.
> 
> On arm64, if the sg_nents() value is used in a call to dma_map_sg() in
> this situation, then it causes a BUG_ON in lib/swiotlb.c because an
> "empty" sg list entry results in dma_capable() returning false and
> swiotlb trying to create a bounce buffer of size 0. This occurred in
> the userspace crypto interface before being fixed by
> 
> 0f477b655a52 ("crypto: algif - Mark sgl end at the end of data")
> 
> Protect against this in the future by counting the number of sg entries
> needed to meet the length requirement and supplying that value to
> dma_map_sg().

Is this needed for any reason other than this bug that's already
been fixed?

The reason I'm asking is because while this patch fixes your driver
everybody else will still crash and burn should something like this
happen again.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH v1 3/3] crypto: ccp - Protect against poorly marked end of sg list

[Herbert Xu]

On Wed, May 27, 2015 at 05:43:05PM +0800, Herbert Xu wrote:
> Tom Lendacky <thomas.lendacky@amd.com> wrote:
> > Scatter gather lists can be created with more available entries than are
> > actually used (e.g. using sg_init_table() to reserve a specific number
> > of sg entries, but in actuality using something less than that based on
> > the data length).  The caller sometimes fails to mark the last entry
> > with sg_mark_end().  In these cases, sg_nents() will return the original
> > size of the sg list as opposed to the actual number of sg entries that
> > contain valid data.
> > 
> > On arm64, if the sg_nents() value is used in a call to dma_map_sg() in
> > this situation, then it causes a BUG_ON in lib/swiotlb.c because an
> > "empty" sg list entry results in dma_capable() returning false and
> > swiotlb trying to create a bounce buffer of size 0. This occurred in
> > the userspace crypto interface before being fixed by
> > 
> > 0f477b655a52 ("crypto: algif - Mark sgl end at the end of data")
> > 
> > Protect against this in the future by counting the number of sg entries
> > needed to meet the length requirement and supplying that value to
> > dma_map_sg().
> 
> Is this needed for any reason other than this bug that's already
> been fixed?

Could this be needed if you have a properly marked SG list say of
100 bytes but len is only 10 bytes?

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH v1 3/3] crypto: ccp - Protect against poorly marked end of sg list

[Tom Lendacky]

On 05/27/2015 04:45 AM, Herbert Xu wrote:
> On Wed, May 27, 2015 at 05:43:05PM +0800, Herbert Xu wrote:
>> Tom Lendacky <thomas.lendacky@amd.com> wrote:
>>> Scatter gather lists can be created with more available entries than are
>>> actually used (e.g. using sg_init_table() to reserve a specific number
>>> of sg entries, but in actuality using something less than that based on
>>> the data length).  The caller sometimes fails to mark the last entry
>>> with sg_mark_end().  In these cases, sg_nents() will return the original
>>> size of the sg list as opposed to the actual number of sg entries that
>>> contain valid data.
>>>
>>> On arm64, if the sg_nents() value is used in a call to dma_map_sg() in
>>> this situation, then it causes a BUG_ON in lib/swiotlb.c because an
>>> "empty" sg list entry results in dma_capable() returning false and
>>> swiotlb trying to create a bounce buffer of size 0. This occurred in
>>> the userspace crypto interface before being fixed by
>>>
>>> 0f477b655a52 ("crypto: algif - Mark sgl end at the end of data")
>>>
>>> Protect against this in the future by counting the number of sg entries
>>> needed to meet the length requirement and supplying that value to
>>> dma_map_sg().
>>
>> Is this needed for any reason other than this bug that's already
>> been fixed?
>
> Could this be needed if you have a properly marked SG list say of
> 100 bytes but len is only 10 bytes?

I don't think that situation matters because the DMA mapping should
succeed just fine at 100 bytes even if only needing/using 10 bytes.

Thanks,
Tom

>
> Cheers,
>

Re: [PATCH v1 3/3] crypto: ccp - Protect against poorly marked end of sg list

[Tom Lendacky]

On 05/27/2015 04:43 AM, Herbert Xu wrote:
> Tom Lendacky <thomas.lendacky@amd.com> wrote:
>> Scatter gather lists can be created with more available entries than are
>> actually used (e.g. using sg_init_table() to reserve a specific number
>> of sg entries, but in actuality using something less than that based on
>> the data length).  The caller sometimes fails to mark the last entry
>> with sg_mark_end().  In these cases, sg_nents() will return the original
>> size of the sg list as opposed to the actual number of sg entries that
>> contain valid data.
>>
>> On arm64, if the sg_nents() value is used in a call to dma_map_sg() in
>> this situation, then it causes a BUG_ON in lib/swiotlb.c because an
>> "empty" sg list entry results in dma_capable() returning false and
>> swiotlb trying to create a bounce buffer of size 0. This occurred in
>> the userspace crypto interface before being fixed by
>>
>> 0f477b655a52 ("crypto: algif - Mark sgl end at the end of data")
>>
>> Protect against this in the future by counting the number of sg entries
>> needed to meet the length requirement and supplying that value to
>> dma_map_sg().
>
> Is this needed for any reason other than this bug that's already
> been fixed?
>

I added this just to protect against any other users of the API that
may do something similar in the future (or if the user should re-use
an sg list and leave leftover sg entries in it). Since software
crypto implementations walk the sg list based on length and do not use
DMA mappings it is possible for this bug to pop up again in another
location since it is likely that the testing won't be done with
hardware crypto devices.

> The reason I'm asking is because while this patch fixes your driver
> everybody else will still crash and burn should something like this
> happen again.

A number of other drivers already have similar sg-count functions in
them.

I'm ok if you decide that this patch shouldn't be applied. It's just
that this is typically an issue that won't be found until after the
release of a kernel rather than during the development stages.

Thanks,
Tom

>
> Cheers,
>

Re: [PATCH v1 3/3] crypto: ccp - Protect against poorly marked end of sg list

[Herbert Xu]

On Wed, May 27, 2015 at 09:12:02AM -0500, Tom Lendacky wrote:
>
> >The reason I'm asking is because while this patch fixes your driver
> >everybody else will still crash and burn should something like this
> >happen again.
> 
> A number of other drivers already have similar sg-count functions in
> them.

Perhaps you can help abstract this into a helper that everybody can
call?

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH v1 3/3] crypto: ccp - Protect against poorly marked end of sg list

[Tom Lendacky]

On 05/27/2015 07:36 PM, Herbert Xu wrote:
> On Wed, May 27, 2015 at 09:12:02AM -0500, Tom Lendacky wrote:
>>
>>> The reason I'm asking is because while this patch fixes your driver
>>> everybody else will still crash and burn should something like this
>>> happen again.
>>
>> A number of other drivers already have similar sg-count functions in
>> them.
>
> Perhaps you can help abstract this into a helper that everybody can
> call?

I can do that.  Something like an sg_nents_for_len() function that takes
an sg pointer and a u64 length as arguments. The function should also
return an error if the length requirement isn't satisfied.

Thanks,
Tom

>
> Cheers,
>

Re: [PATCH v1 0/3] crypto: ccp - CCP driver updates 2015-05-26

[Herbert Xu]

On Tue, May 26, 2015 at 01:06:13PM -0500, Tom Lendacky wrote:
> The following patches are included in this driver update series:
> 
> - Remove the checking and setting of the device dma_mask field
> - Remove an unused field from a structure to help avoid any confusion
> - Protect against poorly marked end of scatter-gather list
>  
> This patch series is based on cryptodev-2.6.

Patches 1 and 2 applied.  I'll wait for your response before
deciding on patch 3.

Thanks,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt