LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH] crypto: chtls - fix a missing-check bug
@ 2018-05-05 18:53 Wenwen Wang
  2018-05-07 10:39 ` Dan Carpenter
  0 siblings, 1 reply; 4+ messages in thread
From: Wenwen Wang @ 2018-05-05 18:53 UTC (permalink / raw)
  To: Wenwen Wang
  Cc: Kangjie Lu, Harsh Jain, Herbert Xu, David S. Miller, Atul Gupta,
	Michael Werner, Casey Leedom,
	open list:CXGB4 CRYPTO DRIVER (chcr),
	open list

In do_chtls_setsockopt(), the tls crypto info is first copied from the
poiner 'optval' in userspace and saved to 'tmp_crypto_info'. Then the
'version' of the crypto info is checked. If the version is not as expected,
i.e., TLS_1_2_VERSION, error code -ENOTSUPP is returned to indicate that
the provided crypto info is not supported yet. Then, the 'cipher_type'
field of the 'tmp_crypto_info' is also checked to see if it is
TLS_CIPHER_AES_GCM_128. If it is, the whole struct of
tls12_crypto_info_aes_gcm_128 is copied from the pointer 'optval' and then
the function chtls_setkey() is invoked to set the key.

Given that the 'optval' pointer resides in userspace, a malicious userspace
process can race to change the data pointed by 'optval' between the two
copies. For example, a user can provide a crypto info with TLS_1_2_VERSION
and TLS_CIPHER_AES_GCM_128. After the first copy, the user can modify the
'version' and the 'cipher_type' fields to any versions and/or cipher types
that are not allowed. This way, the user can bypass the checks, inject
bad data to the kernel, cause chtls_setkey() to set a wrong key or other
issues.

This patch reuses the data copied in the first try so as to ensure these
checks will not be bypassed.

Signed-off-by: Wenwen Wang <wang6495@umn.edu>
---
 drivers/crypto/chelsio/chtls/chtls_main.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/crypto/chelsio/chtls/chtls_main.c b/drivers/crypto/chelsio/chtls/chtls_main.c
index 007c45c..859958a 100644
--- a/drivers/crypto/chelsio/chtls/chtls_main.c
+++ b/drivers/crypto/chelsio/chtls/chtls_main.c
@@ -491,9 +491,13 @@ static int do_chtls_setsockopt(struct sock *sk, int optname,
 
 	switch (tmp_crypto_info.cipher_type) {
 	case TLS_CIPHER_AES_GCM_128: {
-		rc = copy_from_user(crypto_info, optval,
-				    sizeof(struct
-					   tls12_crypto_info_aes_gcm_128));
+		/* Obtain version and type from previous copy */
+		crypto_info[0] = tmp_crypto_info;
+		/* Now copy the following data */
+		rc = copy_from_user(crypto_info + sizeof(*crypto_info),
+				optval + sizeof(*crypto_info),
+				sizeof(struct tls12_crypto_info_aes_gcm_128)
+				- sizeof(*crypto_info));
 
 		if (rc) {
 			rc = -EFAULT;
-- 
2.7.4

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] crypto: chtls - fix a missing-check bug
  2018-05-05 18:53 [PATCH] crypto: chtls - fix a missing-check bug Wenwen Wang
@ 2018-05-07 10:39 ` Dan Carpenter
  0 siblings, 0 replies; 4+ messages in thread
From: Dan Carpenter @ 2018-05-07 10:39 UTC (permalink / raw)
  To: kbuild, Wenwen Wang
  Cc: kbuild-all, Wenwen Wang, Kangjie Lu, Harsh Jain, Herbert Xu,
	David S. Miller, Atul Gupta, Michael Werner, Casey Leedom,
	open list:CXGB4 CRYPTO DRIVER (chcr),
	open list

Hi Wenwen,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on cryptodev/master]
[also build test WARNING on v4.17-rc3 next-20180504]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Wenwen-Wang/crypto-chtls-fix-a-missing-check-bug/20180506-091039
base:   https://git.kernel.org/pub/scm/linux/kernel/git/herbert/cryptodev-2.6.git master
:::::: branch date: 26 hours ago
:::::: commit date: 26 hours ago

New smatch warnings:
drivers/crypto/chelsio/chtls/chtls_main.c:496 do_chtls_setsockopt() warn: potential pointer math issue ('crypto_info' is a 32 bit pointer)

Old smatch warnings:
drivers/crypto/chelsio/chtls/chtls_main.c:253 chtls_uld_add() error: buffer overflow 'cdev->rspq_skb_cache' 32 <= 32
drivers/crypto/chelsio/chtls/chtls_main.c:350 chtls_recv_packet() error: double free of 'skb'
drivers/crypto/chelsio/chtls/chtls_main.c:390 chtls_recv_rsp() error: double free of 'skb'
drivers/crypto/chelsio/chtls/chtls_main.c:408 chtls_recv() error: double free of 'skb'

# https://github.com/0day-ci/linux/commit/183b5e3e71c75e3149dac2698883f0bd63a89c75
git remote add linux-review https://github.com/0day-ci/linux
git remote update linux-review
git checkout 183b5e3e71c75e3149dac2698883f0bd63a89c75
vim +496 drivers/crypto/chelsio/chtls/chtls_main.c

a0894394 Atul Gupta  2018-03-31  461  
a0894394 Atul Gupta  2018-03-31  462  static int do_chtls_setsockopt(struct sock *sk, int optname,
a0894394 Atul Gupta  2018-03-31  463  			       char __user *optval, unsigned int optlen)
a0894394 Atul Gupta  2018-03-31  464  {
a0894394 Atul Gupta  2018-03-31  465  	struct tls_crypto_info *crypto_info, tmp_crypto_info;
a0894394 Atul Gupta  2018-03-31  466  	struct chtls_sock *csk;
a0894394 Atul Gupta  2018-03-31  467  	int keylen;
a0894394 Atul Gupta  2018-03-31  468  	int rc = 0;
a0894394 Atul Gupta  2018-03-31  469  
a0894394 Atul Gupta  2018-03-31  470  	csk = rcu_dereference_sk_user_data(sk);
a0894394 Atul Gupta  2018-03-31  471  
a0894394 Atul Gupta  2018-03-31  472  	if (!optval || optlen < sizeof(*crypto_info)) {
a0894394 Atul Gupta  2018-03-31  473  		rc = -EINVAL;
a0894394 Atul Gupta  2018-03-31  474  		goto out;
a0894394 Atul Gupta  2018-03-31  475  	}
a0894394 Atul Gupta  2018-03-31  476  
a0894394 Atul Gupta  2018-03-31  477  	rc = copy_from_user(&tmp_crypto_info, optval, sizeof(*crypto_info));
a0894394 Atul Gupta  2018-03-31  478  	if (rc) {
a0894394 Atul Gupta  2018-03-31  479  		rc = -EFAULT;
a0894394 Atul Gupta  2018-03-31  480  		goto out;
a0894394 Atul Gupta  2018-03-31  481  	}
a0894394 Atul Gupta  2018-03-31  482  
a0894394 Atul Gupta  2018-03-31  483  	/* check version */
a0894394 Atul Gupta  2018-03-31  484  	if (tmp_crypto_info.version != TLS_1_2_VERSION) {
a0894394 Atul Gupta  2018-03-31  485  		rc = -ENOTSUPP;
a0894394 Atul Gupta  2018-03-31  486  		goto out;
a0894394 Atul Gupta  2018-03-31  487  	}
a0894394 Atul Gupta  2018-03-31  488  
a0894394 Atul Gupta  2018-03-31  489  	crypto_info = (struct tls_crypto_info *)&csk->tlshws.crypto_info;
a0894394 Atul Gupta  2018-03-31  490  
a0894394 Atul Gupta  2018-03-31  491  	switch (tmp_crypto_info.cipher_type) {
a0894394 Atul Gupta  2018-03-31  492  	case TLS_CIPHER_AES_GCM_128: {
183b5e3e Wenwen Wang 2018-05-05  493  		/* Obtain version and type from previous copy */
183b5e3e Wenwen Wang 2018-05-05  494  		crypto_info[0] = tmp_crypto_info;
183b5e3e Wenwen Wang 2018-05-05  495  		/* Now copy the following data */
183b5e3e Wenwen Wang 2018-05-05 @496  		rc = copy_from_user(crypto_info + sizeof(*crypto_info),
183b5e3e Wenwen Wang 2018-05-05  497  				optval + sizeof(*crypto_info),
183b5e3e Wenwen Wang 2018-05-05  498  				sizeof(struct tls12_crypto_info_aes_gcm_128)
183b5e3e Wenwen Wang 2018-05-05  499  				- sizeof(*crypto_info));
a0894394 Atul Gupta  2018-03-31  500  
a0894394 Atul Gupta  2018-03-31  501  		if (rc) {
a0894394 Atul Gupta  2018-03-31  502  			rc = -EFAULT;
a0894394 Atul Gupta  2018-03-31  503  			goto out;
a0894394 Atul Gupta  2018-03-31  504  		}
a0894394 Atul Gupta  2018-03-31  505  
a0894394 Atul Gupta  2018-03-31  506  		keylen = TLS_CIPHER_AES_GCM_128_KEY_SIZE;
a0894394 Atul Gupta  2018-03-31  507  		rc = chtls_setkey(csk, keylen, optname);
a0894394 Atul Gupta  2018-03-31  508  		break;
a0894394 Atul Gupta  2018-03-31  509  	}
a0894394 Atul Gupta  2018-03-31  510  	default:
a0894394 Atul Gupta  2018-03-31  511  		rc = -EINVAL;
a0894394 Atul Gupta  2018-03-31  512  		goto out;
a0894394 Atul Gupta  2018-03-31  513  	}
a0894394 Atul Gupta  2018-03-31  514  out:
a0894394 Atul Gupta  2018-03-31  515  	return rc;
a0894394 Atul Gupta  2018-03-31  516  }
a0894394 Atul Gupta  2018-03-31  517  

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] crypto: chtls - fix a missing-check bug
  2018-05-18 19:55 Wenwen Wang
@ 2018-05-26 16:27 ` Herbert Xu
  0 siblings, 0 replies; 4+ messages in thread
From: Herbert Xu @ 2018-05-26 16:27 UTC (permalink / raw)
  To: Wenwen Wang
  Cc: Kangjie Lu, Harsh Jain, David S. Miller, Atul Gupta,
	Michael Werner, Casey Leedom,
	open list:CXGB4 CRYPTO DRIVER (chcr),
	open list

On Fri, May 18, 2018 at 02:55:35PM -0500, Wenwen Wang wrote:
> In do_chtls_setsockopt(), the tls crypto info is first copied from the
> poiner 'optval' in userspace and saved to 'tmp_crypto_info'. Then the
> 'version' of the crypto info is checked. If the version is not as expected,
> i.e., TLS_1_2_VERSION, error code -ENOTSUPP is returned to indicate that
> the provided crypto info is not supported yet. Then, the 'cipher_type'
> field of the 'tmp_crypto_info' is also checked to see if it is
> TLS_CIPHER_AES_GCM_128. If it is, the whole struct of
> tls12_crypto_info_aes_gcm_128 is copied from the pointer 'optval' and then
> the function chtls_setkey() is invoked to set the key.
> 
> Given that the 'optval' pointer resides in userspace, a malicious userspace
> process can race to change the data pointed by 'optval' between the two
> copies. For example, a user can provide a crypto info with TLS_1_2_VERSION
> and TLS_CIPHER_AES_GCM_128. After the first copy, the user can modify the
> 'version' and the 'cipher_type' fields to any versions and/or cipher types
> that are not allowed. This way, the user can bypass the checks, inject
> bad data to the kernel, cause chtls_setkey() to set a wrong key or other
> issues.
> 
> This patch reuses the data copied in the first try so as to ensure these
> checks will not be bypassed.
> 
> Signed-off-by: Wenwen Wang <wang6495@umn.edu>

Patch applied.  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

^ permalink raw reply	[flat|nested] 4+ messages in thread

* [PATCH] crypto: chtls - fix a missing-check bug
@ 2018-05-18 19:55 Wenwen Wang
  2018-05-26 16:27 ` Herbert Xu
  0 siblings, 1 reply; 4+ messages in thread
From: Wenwen Wang @ 2018-05-18 19:55 UTC (permalink / raw)
  To: Wenwen Wang
  Cc: Kangjie Lu, Harsh Jain, Herbert Xu, David S. Miller, Atul Gupta,
	Michael Werner, Casey Leedom,
	open list:CXGB4 CRYPTO DRIVER (chcr),
	open list

In do_chtls_setsockopt(), the tls crypto info is first copied from the
poiner 'optval' in userspace and saved to 'tmp_crypto_info'. Then the
'version' of the crypto info is checked. If the version is not as expected,
i.e., TLS_1_2_VERSION, error code -ENOTSUPP is returned to indicate that
the provided crypto info is not supported yet. Then, the 'cipher_type'
field of the 'tmp_crypto_info' is also checked to see if it is
TLS_CIPHER_AES_GCM_128. If it is, the whole struct of
tls12_crypto_info_aes_gcm_128 is copied from the pointer 'optval' and then
the function chtls_setkey() is invoked to set the key.

Given that the 'optval' pointer resides in userspace, a malicious userspace
process can race to change the data pointed by 'optval' between the two
copies. For example, a user can provide a crypto info with TLS_1_2_VERSION
and TLS_CIPHER_AES_GCM_128. After the first copy, the user can modify the
'version' and the 'cipher_type' fields to any versions and/or cipher types
that are not allowed. This way, the user can bypass the checks, inject
bad data to the kernel, cause chtls_setkey() to set a wrong key or other
issues.

This patch reuses the data copied in the first try so as to ensure these
checks will not be bypassed.

Signed-off-by: Wenwen Wang <wang6495@umn.edu>
---
 drivers/crypto/chelsio/chtls/chtls_main.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/crypto/chelsio/chtls/chtls_main.c b/drivers/crypto/chelsio/chtls/chtls_main.c
index 007c45c..39aab05 100644
--- a/drivers/crypto/chelsio/chtls/chtls_main.c
+++ b/drivers/crypto/chelsio/chtls/chtls_main.c
@@ -491,9 +491,13 @@ static int do_chtls_setsockopt(struct sock *sk, int optname,
 
 	switch (tmp_crypto_info.cipher_type) {
 	case TLS_CIPHER_AES_GCM_128: {
-		rc = copy_from_user(crypto_info, optval,
-				    sizeof(struct
-					   tls12_crypto_info_aes_gcm_128));
+		/* Obtain version and type from previous copy */
+		crypto_info[0] = tmp_crypto_info;
+		/* Now copy the following data */
+		rc = copy_from_user((char *)crypto_info + sizeof(*crypto_info),
+				optval + sizeof(*crypto_info),
+				sizeof(struct tls12_crypto_info_aes_gcm_128)
+				- sizeof(*crypto_info));
 
 		if (rc) {
 			rc = -EFAULT;
-- 
2.7.4

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2018-05-26 16:27 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-05-05 18:53 [PATCH] crypto: chtls - fix a missing-check bug Wenwen Wang
2018-05-07 10:39 ` Dan Carpenter
2018-05-18 19:55 Wenwen Wang
2018-05-26 16:27 ` Herbert Xu

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).