[PATCH 1/1] crypto: Fix possible buffer overflows in pkey_protkey_aes_attr_read

[PATCH 1/1] crypto: Fix possible buffer overflows in pkey_protkey_aes_attr_read

[Xiaohui Zhang]

From: Zhang Xiaohui <ruc_zhangxiaohui@163.com>

pkey_protkey_aes_attr_read() calls memcpy() without checking the
destination size may trigger a buffer overflower.

Signed-off-by: Zhang Xiaohui <ruc_zhangxiaohui@163.com>
---
 drivers/s390/crypto/pkey_api.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/s390/crypto/pkey_api.c b/drivers/s390/crypto/pkey_api.c
index 99cb60ea6..abc237130 100644
--- a/drivers/s390/crypto/pkey_api.c
+++ b/drivers/s390/crypto/pkey_api.c
@@ -1589,6 +1589,8 @@ static ssize_t pkey_protkey_aes_attr_read(u32 keytype, bool is_xts, char *buf,
 	if (rc)
 		return rc;
 
+	if (protkey.len > MAXPROTKEYSIZE)
+		protkey.len = MAXPROTKEYSIZE;
 	protkeytoken.len = protkey.len;
 	memcpy(&protkeytoken.protkey, &protkey.protkey, protkey.len);
 
@@ -1599,6 +1601,8 @@ static ssize_t pkey_protkey_aes_attr_read(u32 keytype, bool is_xts, char *buf,
 		if (rc)
 			return rc;
 
+		if (protkey.len > MAXPROTKEYSIZE)
+			protkey.len = MAXPROTKEYSIZE;
 		protkeytoken.len = protkey.len;
 		memcpy(&protkeytoken.protkey, &protkey.protkey, protkey.len);
 
-- 
2.17.1

Re: [PATCH 1/1] crypto: Fix possible buffer overflows in pkey_protkey_aes_attr_read

[Christian Borntraeger]

On 09.12.20 07:47, Xiaohui Zhang wrote:
> From: Zhang Xiaohui <ruc_zhangxiaohui@163.com>
> 
> pkey_protkey_aes_attr_read() calls memcpy() without checking the
> destination size may trigger a buffer overflower.

To me it looks like protkey.len is generated programmatically in pkey_genprotkey/pkey_clr2protkey
and this purely depends on the keytype and we do check for known ones.
Not sure how this can happen.

> 
> Signed-off-by: Zhang Xiaohui <ruc_zhangxiaohui@163.com>
> ---
>  drivers/s390/crypto/pkey_api.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/s390/crypto/pkey_api.c b/drivers/s390/crypto/pkey_api.c
> index 99cb60ea6..abc237130 100644
> --- a/drivers/s390/crypto/pkey_api.c
> +++ b/drivers/s390/crypto/pkey_api.c
> @@ -1589,6 +1589,8 @@ static ssize_t pkey_protkey_aes_attr_read(u32 keytype, bool is_xts, char *buf,
>  	if (rc)
>  		return rc;
>  
> +	if (protkey.len > MAXPROTKEYSIZE)
> +		protkey.len = MAXPROTKEYSIZE;
>  	protkeytoken.len = protkey.len;
>  	memcpy(&protkeytoken.protkey, &protkey.protkey, protkey.len);
>  
> @@ -1599,6 +1601,8 @@ static ssize_t pkey_protkey_aes_attr_read(u32 keytype, bool is_xts, char *buf,
>  		if (rc)
>  			return rc;
>  
> +		if (protkey.len > MAXPROTKEYSIZE)
> +			protkey.len = MAXPROTKEYSIZE;
>  		protkeytoken.len = protkey.len;
>  		memcpy(&protkeytoken.protkey, &protkey.protkey, protkey.len);
>  
> 

Re: [PATCH 1/1] crypto: Fix possible buffer overflows in pkey_protkey_aes_attr_read

[Harald Freudenberger]

On 09.12.20 07:47, Xiaohui Zhang wrote:
> From: Zhang Xiaohui <ruc_zhangxiaohui@163.com>
>
> pkey_protkey_aes_attr_read() calls memcpy() without checking the
> destination size may trigger a buffer overflower.
>
> Signed-off-by: Zhang Xiaohui <ruc_zhangxiaohui@163.com>
> ---
>  drivers/s390/crypto/pkey_api.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/drivers/s390/crypto/pkey_api.c b/drivers/s390/crypto/pkey_api.c
> index 99cb60ea6..abc237130 100644
> --- a/drivers/s390/crypto/pkey_api.c
> +++ b/drivers/s390/crypto/pkey_api.c
> @@ -1589,6 +1589,8 @@ static ssize_t pkey_protkey_aes_attr_read(u32 keytype, bool is_xts, char *buf,
>  	if (rc)
>  		return rc;
>  
> +	if (protkey.len > MAXPROTKEYSIZE)
> +		protkey.len = MAXPROTKEYSIZE;
>  	protkeytoken.len = protkey.len;
>  	memcpy(&protkeytoken.protkey, &protkey.protkey, protkey.len);
>  
> @@ -1599,6 +1601,8 @@ static ssize_t pkey_protkey_aes_attr_read(u32 keytype, bool is_xts, char *buf,
>  		if (rc)
>  			return rc;
>  
> +		if (protkey.len > MAXPROTKEYSIZE)
> +			protkey.len = MAXPROTKEYSIZE;
>  		protkeytoken.len = protkey.len;
>  		memcpy(&protkeytoken.protkey, &protkey.protkey, protkey.len);
>  
Thanks Xiaohui
but one rule within the kernel is to trust the other internal functions to do the right thing.
So usually only on entrance into the kernel the api parameters are checked but within the
kernel each function trusts the other and no further parameter check is done. Otherwise
endless checks of input parameters would take place which is killing the performance.
As you can see the protkey object is stored by the function pkey_genprotkey() which is
called just 2 lines above. An internal function the module should trust here. I don't think
there is an additional length check needed here.
However, Thanks for your contribution.
Harald Freudenberger
see this function calls another function in the very same file and