[PATCH 1/2] crypto: ccp - Reduce stack frame size with KASAN

[PATCH 1/2] crypto: ccp - Reduce stack frame size with KASAN

[Arnd Bergmann]

The newly added AES GCM implementation uses one of the largest stack frames
in the kernel, around 1KB on normal 64-bit kernels, and 1.6KB when CONFIG_KASAN
is enabled:

drivers/crypto/ccp/ccp-ops.c: In function 'ccp_run_aes_gcm_cmd':
drivers/crypto/ccp/ccp-ops.c:851:1: error: the frame size of 1632 bytes is larger than 1536 bytes [-Werror=frame-larger-than=]

This is problematic for multiple reasons:

 - The crypto functions are often used in deep call chains, e.g. behind
   mm, fs and dm layers, making it more likely to run into an actual stack
   overflow

 - Using this much stack space is an indicator that the code is not
   written to be as efficient as it could be.

 - While this goes unnoticed at the moment in mainline with the frame size
   warning being disabled when KASAN is in use, I would like to enable
   the warning again, and the current code is slightly above my arbitrary
   pick for a limit of 1536 bytes (I already did patches for every other
   driver exceeding this).

A more drastic refactoring of the driver might be needed to reduce the
stack usage more substantially, but this patch is fairly simple and
at least addresses the third one of the problems I mentioned, reducing the
stack size by about 150 bytes and bringing it below the warning limit
I picked.

Fixes: 36cf515b9bbe ("crypto: ccp - Enable support for AES GCM on v5 CCPs")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/crypto/ccp/ccp-dev.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/crypto/ccp/ccp-dev.h b/drivers/crypto/ccp/ccp-dev.h
index 3a45c2af2fbd..c5ea0796a891 100644
--- a/drivers/crypto/ccp/ccp-dev.h
+++ b/drivers/crypto/ccp/ccp-dev.h
@@ -432,24 +432,24 @@ struct ccp_dma_info {
 	unsigned int offset;
 	unsigned int length;
 	enum dma_data_direction dir;
-};
+} __packed __aligned(4);
 
 struct ccp_dm_workarea {
 	struct device *dev;
 	struct dma_pool *dma_pool;
-	unsigned int length;
 
 	u8 *address;
 	struct ccp_dma_info dma;
+	unsigned int length;
 };
 
 struct ccp_sg_workarea {
 	struct scatterlist *sg;
 	int nents;
+	unsigned int dma_count;
 
 	struct scatterlist *dma_sg;
 	struct device *dma_dev;
-	unsigned int dma_count;
 	enum dma_data_direction dma_dir;
 
 	unsigned int sg_used;
-- 
2.9.0

[PATCH 2/2] crypto: ccp - Mark driver as little-endian only

[Arnd Bergmann]

The driver causes a warning when built as big-endian:

drivers/crypto/ccp/ccp-dev-v5.c: In function 'ccp5_perform_des3':
include/uapi/linux/byteorder/big_endian.h:32:26: error: large integer implicitly truncated to unsigned type [-Werror=overflow]
 #define __cpu_to_le32(x) ((__force __le32)__swab32((x)))
                          ^
include/linux/byteorder/generic.h:87:21: note: in expansion of macro '__cpu_to_le32'
 #define cpu_to_le32 __cpu_to_le32
                     ^~~~~~~~~~~~~
drivers/crypto/ccp/ccp-dev-v5.c:436:28: note: in expansion of macro 'cpu_to_le32'
  CCP5_CMD_KEY_MEM(&desc) = cpu_to_le32(CCP_MEMTYPE_SB);

The warning is correct, doing a 32-bit byte swap on a value that gets
assigned into a bit field cannot work, since we would only write zeroes
in this case, regardless of the input.

In fact, the use of bit fields in hardware defined data structures is
not portable to start with, so until all these bit fields get replaced
by something else, the driver cannot work on big-endian machines, and
I'm adding an annotation here to prevent it from being selected.

The CCPv3 code seems to not suffer from this problem, only v5 uses
bitfields.

Fixes: 4b394a232df7 ("crypto: ccp - Let a v5 CCP provide the same function as v3")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/crypto/ccp/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/crypto/ccp/Kconfig b/drivers/crypto/ccp/Kconfig
index 2238f77aa248..07af9ece84f9 100644
--- a/drivers/crypto/ccp/Kconfig
+++ b/drivers/crypto/ccp/Kconfig
@@ -1,6 +1,7 @@
 config CRYPTO_DEV_CCP_DD
 	tristate "Cryptographic Coprocessor device driver"
 	depends on CRYPTO_DEV_CCP
+	depends on !CPU_BIG_ENDIAN || BROKEN
 	default m
 	select HW_RANDOM
 	select DMA_ENGINE
-- 
2.9.0

Re: [PATCH 2/2] crypto: ccp - Mark driver as little-endian only

[Gary R Hook]

Ack. Didn't reply all....  Sorry, Arnd.

There was a krobot warning about this and I submitted a patch just now.

(I thought) my mistake was (in this function) not handling the structure
elements in the same manner as other functions. My patch rectifies that.

On 03/28/2017 04:58 AM, Arnd Bergmann wrote:
> The driver causes a warning when built as big-endian:
>
> drivers/crypto/ccp/ccp-dev-v5.c: In function 'ccp5_perform_des3':
> include/uapi/linux/byteorder/big_endian.h:32:26: error: large integer
> implicitly truncated to unsigned type [-Werror=overflow]
>  #define __cpu_to_le32(x) ((__force __le32)__swab32((x)))
>                           ^
> include/linux/byteorder/generic.h:87:21: note: in expansion of macro
> '__cpu_to_le32'
>  #define cpu_to_le32 __cpu_to_le32
>                      ^~~~~~~~~~~~~
> drivers/crypto/ccp/ccp-dev-v5.c:436:28: note: in expansion of macro
> 'cpu_to_le32'
>   CCP5_CMD_KEY_MEM(&desc) = cpu_to_le32(CCP_MEMTYPE_SB);
>
> The warning is correct, doing a 32-bit byte swap on a value that gets
> assigned into a bit field cannot work, since we would only write zeroes
> in this case, regardless of the input.

Yes, this was all wrong.

> In fact, the use of bit fields in hardware defined data structures is
> not portable to start with, so until all these bit fields get replaced
> by something else, the driver cannot work on big-endian machines, and
> I'm adding an annotation here to prevent it from being selected.

This is a driver that talks to hardware, a device which, AFAIK, has no
plan to be implemented in a big endian flavor. I clearly need to be more
diligent in building with various checkers enabled. I'd prefer my fix
over your suggested refusal to compile, if that's okay.

> The CCPv3 code seems to not suffer from this problem, only v5 uses
> bitfields.

Yes, I took a different approach when I wrote the code. IMO (arguably)
more readable. Same result: words full of hardware-dependent bit patterns.

Please help me understand what I could do better.

-- 
This is my day job. Follow me at:
IG/Twitter/Facebook: @grhookphoto
IG/Twitter/Facebook: @grhphotographer

Re: [PATCH 1/2] crypto: ccp - Reduce stack frame size with KASAN

[Gary R Hook]

On 03/28/2017 04:58 AM, Arnd Bergmann wrote:> The newly added AES GCM 
implementation uses one of the largest stack frames
> in the kernel, around 1KB on normal 64-bit kernels, and 1.6KB when
> CONFIG_KASAN
> is enabled:
>
> drivers/crypto/ccp/ccp-ops.c: In function 'ccp_run_aes_gcm_cmd':
> drivers/crypto/ccp/ccp-ops.c:851:1: error: the frame size of 1632 bytes
> is larger than 1536 bytes [-Werror=frame-larger-than=]
>
> This is problematic for multiple reasons:
>
>  - The crypto functions are often used in deep call chains, e.g. behind
>    mm, fs and dm layers, making it more likely to run into an actual stack
>    overflow
>
>  - Using this much stack space is an indicator that the code is not
>    written to be as efficient as it could be.

I'm not sure I agree that A -> B, but I will certainly look into this.

>  - While this goes unnoticed at the moment in mainline with the frame size
>    warning being disabled when KASAN is in use, I would like to enable
>    the warning again, and the current code is slightly above my arbitrary
>    pick for a limit of 1536 bytes (I already did patches for every other
>    driver exceeding this).

I've got my stack frame size (also) set to 1536, and would have paid 
more attention
had a warning occurred due to my code.

> A more drastic refactoring of the driver might be needed to reduce the
> stack usage more substantially, but this patch is fairly simple and
> at least addresses the third one of the problems I mentioned, reducing the
> stack size by about 150 bytes and bringing it below the warning limit
> I picked.

Again, I'll devote some time to this.

> diff --git a/drivers/crypto/ccp/ccp-dev.h b/drivers/crypto/ccp/ccp-dev.h
> index 3a45c2af2fbd..c5ea0796a891 100644
> --- a/drivers/crypto/ccp/ccp-dev.h
> +++ b/drivers/crypto/ccp/ccp-dev.h
> @@ -432,24 +432,24 @@ struct ccp_dma_info {
>          unsigned int offset;
>          unsigned int length;
>          enum dma_data_direction dir;
> -};
> +} __packed __aligned(4);

My gcc 4.8 doesn't understand __aligned(). Shouldn't we use
#pragma(4) here?


>  struct ccp_dm_workarea {
>          struct device *dev;
>          struct dma_pool *dma_pool;
> -       unsigned int length;
>
>          u8 *address;
>          struct ccp_dma_info dma;
> +       unsigned int length;
>  };
>
>  struct ccp_sg_workarea {
>          struct scatterlist *sg;
>          int nents;
> +       unsigned int dma_count;
>
>          struct scatterlist *dma_sg;
>          struct device *dma_dev;
> -       unsigned int dma_count;
>          enum dma_data_direction dma_dir;
>
>          unsigned int sg_used;

I'm okay with rearranging, but I'm going to submit an alternative patch.

Re: [PATCH 2/2] crypto: ccp - Mark driver as little-endian only

[Arnd Bergmann]

On Tue, Mar 28, 2017 at 4:08 PM, Gary R Hook <ghook@amd.com> wrote:

>> In fact, the use of bit fields in hardware defined data structures is
>> not portable to start with, so until all these bit fields get replaced
>> by something else, the driver cannot work on big-endian machines, and
>> I'm adding an annotation here to prevent it from being selected.
>
>
> This is a driver that talks to hardware, a device which, AFAIK, has no
> plan to be implemented in a big endian flavor. I clearly need to be more
> diligent in building with various checkers enabled. I'd prefer my fix
> over your suggested refusal to compile, if that's okay.

It's hard to predict the future. If this device ever makes it into an
ARM based chip, the chances are relatively high that someone
will eventually run a big-endian kernel on it. As long as it's guaranteed
to be x86-only, the risk of anyone running into the bug is close to
zero, but we normally still try to write device drivers in portable C
code to prevent it from getting copied incorrectly into another driver.

>> The CCPv3 code seems to not suffer from this problem, only v5 uses
>> bitfields.
>
>
> Yes, I took a different approach when I wrote the code. IMO (arguably)
> more readable. Same result: words full of hardware-dependent bit patterns.
>
> Please help me understand what I could do better.

The rule for portable drivers is that you must not use bitfields in structures
that can be accessed by the hardware. I think you can do this in a more
readable way by removing the CCP5_CMD_* macros etc completely
and just accessing the members of the structure as __le32 words.
The main advantage for readability here is that you can grep for the
struct members and see where they are used without following the
macros. If it helps, you can also encapsulate the generation of the
word inside of an inline function, like:

static inline __le32 ccp5_cmd_dw0(bool soc, bool ioc, bool init, bool
eom, u32 engine)
{
        u32 dw0 = (soc  ? CCP5_WORD0_SOC  : 0)  |
                  (ioc  ? CCP5_WORD0_IOC  : 0)  |
                  (init ? CCP5_WORD0_INIT : 0)  |
                  (eom  ? CCP5_WORD0_EOM  : 0)  |
                CCP5_WORD0_ENGINE(engine);

        return __cpu_to_le32(dw0);
}

...
desc->dw0 = ccp5_cmd_dw0(op->soc, 0, op->init, op->oem, op->engine);

       Arnd

Re: [PATCH 1/2] crypto: ccp - Reduce stack frame size with KASAN

[Arnd Bergmann]

On Tue, Mar 28, 2017 at 4:15 PM, Gary R Hook <ghook@amd.com> wrote:

>> A more drastic refactoring of the driver might be needed to reduce the
>> stack usage more substantially, but this patch is fairly simple and
>> at least addresses the third one of the problems I mentioned, reducing the
>> stack size by about 150 bytes and bringing it below the warning limit
>> I picked.
>
>
> Again, I'll devote some time to this.
>
>> diff --git a/drivers/crypto/ccp/ccp-dev.h b/drivers/crypto/ccp/ccp-dev.h
>> index 3a45c2af2fbd..c5ea0796a891 100644
>> --- a/drivers/crypto/ccp/ccp-dev.h
>> +++ b/drivers/crypto/ccp/ccp-dev.h
>> @@ -432,24 +432,24 @@ struct ccp_dma_info {
>>          unsigned int offset;
>>          unsigned int length;
>>          enum dma_data_direction dir;
>> -};
>> +} __packed __aligned(4);
>
>
> My gcc 4.8 doesn't understand __aligned(). Shouldn't we use
> #pragma(4) here?



>>  struct ccp_dm_workarea {
>>          struct device *dev;
>>          struct dma_pool *dma_pool;
>> -       unsigned int length;
>>
>>          u8 *address;
>>          struct ccp_dma_info dma;
>> +       unsigned int length;
>>  };
>>
>>  struct ccp_sg_workarea {
>>          struct scatterlist *sg;
>>          int nents;
>> +       unsigned int dma_count;
>>
>>          struct scatterlist *dma_sg;
>>          struct device *dma_dev;
>> -       unsigned int dma_count;
>>          enum dma_data_direction dma_dir;
>>
>>          unsigned int sg_used;
>
>
> I'm okay with rearranging, but I'm going to submit an alternative patch.

Ok, thanks a lot!

Re: [PATCH 1/2] crypto: ccp - Reduce stack frame size with KASAN

[Arnd Bergmann]

On Tue, Mar 28, 2017 at 4:15 PM, Gary R Hook <ghook@amd.com> wrote:
> On 03/28/2017 04:58 AM, Arnd Bergmann wrote:> The newly added AES GCM
> implementation uses one of the largest stack frames

>> diff --git a/drivers/crypto/ccp/ccp-dev.h b/drivers/crypto/ccp/ccp-dev.h
>> index 3a45c2af2fbd..c5ea0796a891 100644
>> --- a/drivers/crypto/ccp/ccp-dev.h
>> +++ b/drivers/crypto/ccp/ccp-dev.h
>> @@ -432,24 +432,24 @@ struct ccp_dma_info {
>>          unsigned int offset;
>>          unsigned int length;
>>          enum dma_data_direction dir;
>> -};
>> +} __packed __aligned(4);
>
>
> My gcc 4.8 doesn't understand __aligned(). Shouldn't we use
> #pragma(4) here?

That is odd, the __aligned() macro gets defined for all compiler versions
in linux/compiler.h, and the aligned attribute should work for all supported
compilers (3.2 and higher), while #pragma pack() requires gcc-4.0 or
higher.

We generally prefer attribute syntax in the kernel over pragmas, even
when they are functionally the same.

      Arnd

Re: [PATCH 2/2] crypto: ccp - Mark driver as little-endian only

[Gary R Hook]

On 03/28/2017 09:59 AM, Arnd Bergmann wrote:
> On Tue, Mar 28, 2017 at 4:08 PM, Gary R Hook <ghook@amd.com> wrote:
>
>>> In fact, the use of bit fields in hardware defined data structures is
>>> not portable to start with, so until all these bit fields get replaced
>>> by something else, the driver cannot work on big-endian machines, and
>>> I'm adding an annotation here to prevent it from being selected.
>>
>>
>> This is a driver that talks to hardware, a device which, AFAIK, has no
>> plan to be implemented in a big endian flavor. I clearly need to be more
>> diligent in building with various checkers enabled. I'd prefer my fix
>> over your suggested refusal to compile, if that's okay.
>
> It's hard to predict the future. If this device ever makes it into an
> ARM based chip, the chances are relatively high that someone
> will eventually run a big-endian kernel on it. As long as it's guaranteed
> to be x86-only, the risk of anyone running into the bug is close to
> zero, but we normally still try to write device drivers in portable C
> code to prevent it from getting copied incorrectly into another driver.

Understood, and I had surmised as such. Totally agree.

>>> The CCPv3 code seems to not suffer from this problem, only v5 uses
>>> bitfields.
>>
>>
>> Yes, I took a different approach when I wrote the code. IMO (arguably)
>> more readable. Same result: words full of hardware-dependent bit patterns.
>>
>> Please help me understand what I could do better.
>
> The rule for portable drivers is that you must not use bitfields in
> structures
> that can be accessed by the hardware. I think you can do this in a more
> readable way by removing the CCP5_CMD_* macros etc completely
> and just accessing the members of the structure as __le32 words.
> The main advantage for readability here is that you can grep for the
> struct members and see where they are used without following the
> macros. If it helps, you can also encapsulate the generation of the
> word inside of an inline function, like:
>

Please see my follow-on patch.

-- 
This is my day job. Follow me at:
IG/Twitter/Facebook: @grhookphoto
IG/Twitter/Facebook: @grhphotographer

Re: [PATCH 1/2] crypto: ccp - Reduce stack frame size with KASAN

[Gary R Hook]

On 03/28/2017 10:10 AM, Arnd Bergmann wrote:
> On Tue, Mar 28, 2017 at 4:15 PM, Gary R Hook <ghook@amd.com> wrote:
>> On 03/28/2017 04:58 AM, Arnd Bergmann wrote:> The newly added AES GCM
>> implementation uses one of the largest stack frames
>
>>> diff --git a/drivers/crypto/ccp/ccp-dev.h b/drivers/crypto/ccp/ccp-dev.h
>>> index 3a45c2af2fbd..c5ea0796a891 100644
>>> --- a/drivers/crypto/ccp/ccp-dev.h
>>> +++ b/drivers/crypto/ccp/ccp-dev.h
>>> @@ -432,24 +432,24 @@ struct ccp_dma_info {
>>>          unsigned int offset;
>>>          unsigned int length;
>>>          enum dma_data_direction dir;
>>> -};
>>> +} __packed __aligned(4);
>>
>>
>> My gcc 4.8 doesn't understand __aligned(). Shouldn't we use
>> #pragma(4) here?
>
> That is odd, the __aligned() macro gets defined for all compiler versions
> in linux/compiler.h, and the aligned attribute should work for all supported
> compilers (3.2 and higher), while #pragma pack() requires gcc-4.0 or
> higher.
>
> We generally prefer attribute syntax in the kernel over pragmas, even
> when they are functionally the same.

Yes, it's extremely odd, and I understand this preference. Please ignore my
submitted alternate and let me track this down....


-- 
This is my day job. Follow me at:
IG/Twitter/Facebook: @grhookphoto
IG/Twitter/Facebook: @grhphotographer

Re: [PATCH 1/2] crypto: ccp - Reduce stack frame size with KASAN

[Gary R Hook]

On 03/28/2017 10:10 AM, Arnd Bergmann wrote:
>>> -};
>>> +} __packed __aligned(4);
>>
>> My gcc 4.8 doesn't understand __aligned(). Shouldn't we use
>> #pragma(4) here?
>
> That is odd, the __aligned() macro gets defined for all compiler versions
> in linux/compiler.h, and the aligned attribute should work for all supported
> compilers (3.2 and higher), while #pragma pack() requires gcc-4.0 or
> higher.
>

Tried again in a couple of trees. Not sure what I did wrong, but the 
compiler
seems to be happy now. Huh.

Will submit a V2.


-- 
This is my day job. Follow me at:
IG/Twitter/Facebook: @grhookphoto
IG/Twitter/Facebook: @grhphotographer