linux-kernel - Re: [PATCH 1/1] crypto: Fix possible buffer overflows in pkey_protkey_aes_attr

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <601cf44e-1de4-4550-2bad-8087092bd010@linux.ibm.com>
Date:   Wed, 9 Dec 2020 08:46:16 +0100
From:   Harald Freudenberger <freude@...ux.ibm.com>
To:     Xiaohui Zhang <ruc_zhangxiaohui@....com>,
        linux-s390@...r.kernel.org, linux-kernel@...r.kernel.org
Cc:     Heiko Carstens <hca@...ux.ibm.com>,
        Vasily Gorbik <gor@...ux.ibm.com>,
        Christian Borntraeger <borntraeger@...ibm.com>
Subject: Re: [PATCH 1/1] crypto: Fix possible buffer overflows in
 pkey_protkey_aes_attr_read

On 09.12.20 07:47, Xiaohui Zhang wrote:
> From: Zhang Xiaohui <ruc_zhangxiaohui@....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@....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