From: Dan Carpenter <dan.carpenter@oracle.com>
To: Jon Maloy <jmaloy@redhat.com>
Cc: Ying Xue <ying.xue@windriver.com>,
"David S. Miller" <davem@davemloft.net>,
Jakub Kicinski <kuba@kernel.org>,
Tuong Lien <tuong.t.lien@dektech.com.au>,
netdev@vger.kernel.org, tipc-discussion@lists.sourceforge.net,
kernel-janitors@vger.kernel.org,
Julia Lawall <Julia.Lawall@lip6.fr>,
Kees Cook <keescook@chromium.org>
Subject: [PATCH net-next] tipc: potential memory corruption in tipc_crypto_key_rcv()
Date: Wed, 23 Sep 2020 08:30:17 +0000 [thread overview]
Message-ID: <20200923083017.GA1454948@mwanda> (raw)
This code uses "skey->keylen" as an memcpy() size and then checks that
it is valid on the next line. The other problem is that the check has
a potential integer overflow, it's better to use struct_size() for this.
Fixes: 23700da29b83 ("tipc: add automatic rekeying for encryption key")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
Hey Kees and Julia,
It would be nice to change tipc_aead_key_size() but I'm not sure how the
UAPI stuff works. My first attempt at to change it to
return struct_size(key, key, key->keylen);
broke the build. I think you guys used Coccinelle to automatically
update these calculations. Probably this wasn't updated because you
didn't want to break the build either?
net/tipc/crypto.c | 14 ++++++--------
1 file changed, 6 insertions(+), 8 deletions(-)
diff --git a/net/tipc/crypto.c b/net/tipc/crypto.c
index 40c44101fe8e..291ba276b835 100644
--- a/net/tipc/crypto.c
+++ b/net/tipc/crypto.c
@@ -2281,6 +2281,7 @@ static bool tipc_crypto_key_rcv(struct tipc_crypto *rx, struct tipc_msg *hdr)
u16 key_gen = msg_key_gen(hdr);
u16 size = msg_data_sz(hdr);
u8 *data = msg_data(hdr);
+ u32 keylen;
spin_lock(&rx->lock);
if (unlikely(rx->skey || (key_gen = rx->key_gen && rx->key.keys))) {
@@ -2289,6 +2290,10 @@ static bool tipc_crypto_key_rcv(struct tipc_crypto *rx, struct tipc_msg *hdr)
goto exit;
}
+ keylen = ntohl(*((__be32 *)(data + TIPC_AEAD_ALG_NAME)));
+ if (struct_size(skey, key, keylen) != size)
+ goto exit;
+
/* Allocate memory for the key */
skey = kmalloc(size, GFP_ATOMIC);
if (unlikely(!skey)) {
@@ -2297,18 +2302,11 @@ static bool tipc_crypto_key_rcv(struct tipc_crypto *rx, struct tipc_msg *hdr)
}
/* Copy key from msg data */
- skey->keylen = ntohl(*((__be32 *)(data + TIPC_AEAD_ALG_NAME)));
+ skey->keylen = keylen;
memcpy(skey->alg_name, data, TIPC_AEAD_ALG_NAME);
memcpy(skey->key, data + TIPC_AEAD_ALG_NAME + sizeof(__be32),
skey->keylen);
- /* Sanity check */
- if (unlikely(size != tipc_aead_key_size(skey))) {
- kfree(skey);
- skey = NULL;
- goto exit;
- }
-
rx->key_gen = key_gen;
rx->skey_mode = msg_key_mode(hdr);
rx->skey = skey;
--
2.28.0
WARNING: multiple messages have this Message-ID (diff)
From: Dan Carpenter <dan.carpenter@oracle.com>
To: Jon Maloy <jmaloy@redhat.com>
Cc: Ying Xue <ying.xue@windriver.com>,
"David S. Miller" <davem@davemloft.net>,
Jakub Kicinski <kuba@kernel.org>,
Tuong Lien <tuong.t.lien@dektech.com.au>,
netdev@vger.kernel.org, tipc-discussion@lists.sourceforge.net,
kernel-janitors@vger.kernel.org,
Julia Lawall <Julia.Lawall@lip6.fr>,
Kees Cook <keescook@chromium.org>
Subject: [PATCH net-next] tipc: potential memory corruption in tipc_crypto_key_rcv()
Date: Wed, 23 Sep 2020 11:30:17 +0300 [thread overview]
Message-ID: <20200923083017.GA1454948@mwanda> (raw)
This code uses "skey->keylen" as an memcpy() size and then checks that
it is valid on the next line. The other problem is that the check has
a potential integer overflow, it's better to use struct_size() for this.
Fixes: 23700da29b83 ("tipc: add automatic rekeying for encryption key")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
Hey Kees and Julia,
It would be nice to change tipc_aead_key_size() but I'm not sure how the
UAPI stuff works. My first attempt at to change it to
return struct_size(key, key, key->keylen);
broke the build. I think you guys used Coccinelle to automatically
update these calculations. Probably this wasn't updated because you
didn't want to break the build either?
net/tipc/crypto.c | 14 ++++++--------
1 file changed, 6 insertions(+), 8 deletions(-)
diff --git a/net/tipc/crypto.c b/net/tipc/crypto.c
index 40c44101fe8e..291ba276b835 100644
--- a/net/tipc/crypto.c
+++ b/net/tipc/crypto.c
@@ -2281,6 +2281,7 @@ static bool tipc_crypto_key_rcv(struct tipc_crypto *rx, struct tipc_msg *hdr)
u16 key_gen = msg_key_gen(hdr);
u16 size = msg_data_sz(hdr);
u8 *data = msg_data(hdr);
+ u32 keylen;
spin_lock(&rx->lock);
if (unlikely(rx->skey || (key_gen == rx->key_gen && rx->key.keys))) {
@@ -2289,6 +2290,10 @@ static bool tipc_crypto_key_rcv(struct tipc_crypto *rx, struct tipc_msg *hdr)
goto exit;
}
+ keylen = ntohl(*((__be32 *)(data + TIPC_AEAD_ALG_NAME)));
+ if (struct_size(skey, key, keylen) != size)
+ goto exit;
+
/* Allocate memory for the key */
skey = kmalloc(size, GFP_ATOMIC);
if (unlikely(!skey)) {
@@ -2297,18 +2302,11 @@ static bool tipc_crypto_key_rcv(struct tipc_crypto *rx, struct tipc_msg *hdr)
}
/* Copy key from msg data */
- skey->keylen = ntohl(*((__be32 *)(data + TIPC_AEAD_ALG_NAME)));
+ skey->keylen = keylen;
memcpy(skey->alg_name, data, TIPC_AEAD_ALG_NAME);
memcpy(skey->key, data + TIPC_AEAD_ALG_NAME + sizeof(__be32),
skey->keylen);
- /* Sanity check */
- if (unlikely(size != tipc_aead_key_size(skey))) {
- kfree(skey);
- skey = NULL;
- goto exit;
- }
-
rx->key_gen = key_gen;
rx->skey_mode = msg_key_mode(hdr);
rx->skey = skey;
--
2.28.0
next reply other threads:[~2020-09-23 8:30 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-09-23 8:30 Dan Carpenter [this message]
2020-09-23 8:30 ` [PATCH net-next] tipc: potential memory corruption in tipc_crypto_key_rcv() Dan Carpenter
2020-09-24 1:06 ` David Miller
2020-09-24 1:06 ` David Miller
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200923083017.GA1454948@mwanda \
--to=dan.carpenter@oracle.com \
--cc=Julia.Lawall@lip6.fr \
--cc=davem@davemloft.net \
--cc=jmaloy@redhat.com \
--cc=keescook@chromium.org \
--cc=kernel-janitors@vger.kernel.org \
--cc=kuba@kernel.org \
--cc=netdev@vger.kernel.org \
--cc=tipc-discussion@lists.sourceforge.net \
--cc=tuong.t.lien@dektech.com.au \
--cc=ying.xue@windriver.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.