From: Waiman Long <longman@redhat.com> To: David Howells <dhowells@redhat.com>, Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>, James Morris <jmorris@namei.org>, "Serge E. Hallyn" <serge@hallyn.com>, Mimi Zohar <zohar@linux.ibm.com>, "David S. Miller" <davem@davemloft.net>, Jakub Kicinski <kuba@kernel.org> Cc: keyrings@vger.kernel.org, linux-kernel@vger.kernel.org, linux-security-module@vger.kernel.org, linux-integrity@vger.kernel.org, netdev@vger.kernel.org, linux-afs@lists.infradead.org, Sumit Garg <sumit.garg@linaro.org>, Jerry Snitselaar <jsnitsel@redhat.com>, Roberto Sassu <roberto.sassu@huawei.com>, Eric Biggers <ebiggers@google.com>, Chris von Recklinghausen <crecklin@redhat.com>, Waiman Long <longman@redhat.com> Subject: [PATCH v6 2/2] KEYS: Avoid false positive ENOMEM error on key read Date: Fri, 20 Mar 2020 19:19:03 +0000 [thread overview] Message-ID: <20200320191903.19494-3-longman@redhat.com> (raw) In-Reply-To: <20200320191903.19494-1-longman@redhat.com> By allocating a kernel buffer with a user-supplied buffer length, it is possible that a false positive ENOMEM error may be returned because the user-supplied length is just too large even if the system do have enough memory to hold the actual key data. Moreover, if the buffer length is larger than the maximum amount of memory that can be returned by kmalloc() (2^(MAX_ORDER-1) number of pages), a warning message will also be printed. To reduce this possibility, we set a threshold (page size) over which we do check the actual key length first before allocating a buffer of the right size to hold it. The threshold is arbitrary, it is just used to trigger a buffer length check. It does not limit the actual key length as long as there is enough memory to satisfy the memory request. To further avoid large buffer allocation failure due to page fragmentation, kvmalloc() is used to allocate the buffer so that vmapped pages can be used when there is not a large enough contiguous set of pages available for allocation. Signed-off-by: Waiman Long <longman@redhat.com> --- security/keys/internal.h | 12 ++++++++++++ security/keys/keyctl.c | 39 +++++++++++++++++++++++++++++++-------- 2 files changed, 43 insertions(+), 8 deletions(-) diff --git a/security/keys/internal.h b/security/keys/internal.h index ba3e2da14cef..6d0ca48ae9a5 100644 --- a/security/keys/internal.h +++ b/security/keys/internal.h @@ -16,6 +16,8 @@ #include <linux/keyctl.h> #include <linux/refcount.h> #include <linux/compat.h> +#include <linux/mm.h> +#include <linux/vmalloc.h> struct iovec; @@ -349,4 +351,14 @@ static inline void key_check(const struct key *key) #endif +/* + * Helper function to clear and free a kvmalloc'ed memory object. + */ +static inline void __kvzfree(const void *addr, size_t len) +{ + if (addr) { + memset((void *)addr, 0, len); + kvfree(addr); + } +} #endif /* _INTERNAL_H */ diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c index 5a0794cb8815..ded69108db0d 100644 --- a/security/keys/keyctl.c +++ b/security/keys/keyctl.c @@ -339,7 +339,7 @@ long keyctl_update_key(key_serial_t id, payload = NULL; if (plen) { ret = -ENOMEM; - payload = kmalloc(plen, GFP_KERNEL); + payload = kvmalloc(plen, GFP_KERNEL); if (!payload) goto error; @@ -360,7 +360,7 @@ long keyctl_update_key(key_serial_t id, key_ref_put(key_ref); error2: - kzfree(payload); + __kvzfree(payload, plen); error: return ret; } @@ -877,13 +877,23 @@ long keyctl_read_key(key_serial_t keyid, char __user *buffer, size_t buflen) * transferring them to user buffer to avoid potential * deadlock involving page fault and mmap_sem. */ - char *key_data = kmalloc(buflen, GFP_KERNEL); + char *key_data = NULL; + size_t key_data_len = buflen; - if (!key_data) { - ret = -ENOMEM; - goto error2; + /* + * When the user-supplied key length is larger than + * PAGE_SIZE, we get the actual key length first before + * allocating a right-sized key data buffer. + */ + if (buflen <= PAGE_SIZE) { +allocbuf: + key_data = kvmalloc(key_data_len, GFP_KERNEL); + if (!key_data) { + ret = -ENOMEM; + goto error2; + } } - ret = __keyctl_read_key(key, key_data, buflen); + ret = __keyctl_read_key(key, key_data, key_data_len); /* * Read methods will just return the required length @@ -891,10 +901,23 @@ long keyctl_read_key(key_serial_t keyid, char __user *buffer, size_t buflen) * enough. */ if (ret > 0 && ret <= buflen) { + /* + * The key may change (unlikely) in between 2 + * consecutive __keyctl_read_key() calls. We will + * need to allocate a larger buffer and redo the key + * read when key_data_len < ret <= buflen. + */ + if (!key_data || unlikely(ret > key_data_len)) { + if (unlikely(key_data)) + __kvzfree(key_data, key_data_len); + key_data_len = ret; + goto allocbuf; + } + if (copy_to_user(buffer, key_data, ret)) ret = -EFAULT; } - kzfree(key_data); + __kvzfree(key_data, key_data_len); } error2: -- 2.18.1
WARNING: multiple messages have this Message-ID (diff)
From: Waiman Long <longman@redhat.com> To: David Howells <dhowells@redhat.com>, Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>, James Morris <jmorris@namei.org>, "Serge E. Hallyn" <serge@hallyn.com>, Mimi Zohar <zohar@linux.ibm.com>, "David S. Miller" <davem@davemloft.net>, Jakub Kicinski <kuba@kernel.org> Cc: keyrings@vger.kernel.org, linux-kernel@vger.kernel.org, linux-security-module@vger.kernel.org, linux-integrity@vger.kernel.org, netdev@vger.kernel.org, linux-afs@lists.infradead.org, Sumit Garg <sumit.garg@linaro.org>, Jerry Snitselaar <jsnitsel@redhat.com>, Roberto Sassu <roberto.sassu@huawei.com>, Eric Biggers <ebiggers@google.com>, Chris von Recklinghausen <crecklin@redhat.com>, Waiman Long <longman@redhat.com> Subject: [PATCH v6 2/2] KEYS: Avoid false positive ENOMEM error on key read Date: Fri, 20 Mar 2020 15:19:03 -0400 [thread overview] Message-ID: <20200320191903.19494-3-longman@redhat.com> (raw) In-Reply-To: <20200320191903.19494-1-longman@redhat.com> By allocating a kernel buffer with a user-supplied buffer length, it is possible that a false positive ENOMEM error may be returned because the user-supplied length is just too large even if the system do have enough memory to hold the actual key data. Moreover, if the buffer length is larger than the maximum amount of memory that can be returned by kmalloc() (2^(MAX_ORDER-1) number of pages), a warning message will also be printed. To reduce this possibility, we set a threshold (page size) over which we do check the actual key length first before allocating a buffer of the right size to hold it. The threshold is arbitrary, it is just used to trigger a buffer length check. It does not limit the actual key length as long as there is enough memory to satisfy the memory request. To further avoid large buffer allocation failure due to page fragmentation, kvmalloc() is used to allocate the buffer so that vmapped pages can be used when there is not a large enough contiguous set of pages available for allocation. Signed-off-by: Waiman Long <longman@redhat.com> --- security/keys/internal.h | 12 ++++++++++++ security/keys/keyctl.c | 39 +++++++++++++++++++++++++++++++-------- 2 files changed, 43 insertions(+), 8 deletions(-) diff --git a/security/keys/internal.h b/security/keys/internal.h index ba3e2da14cef..6d0ca48ae9a5 100644 --- a/security/keys/internal.h +++ b/security/keys/internal.h @@ -16,6 +16,8 @@ #include <linux/keyctl.h> #include <linux/refcount.h> #include <linux/compat.h> +#include <linux/mm.h> +#include <linux/vmalloc.h> struct iovec; @@ -349,4 +351,14 @@ static inline void key_check(const struct key *key) #endif +/* + * Helper function to clear and free a kvmalloc'ed memory object. + */ +static inline void __kvzfree(const void *addr, size_t len) +{ + if (addr) { + memset((void *)addr, 0, len); + kvfree(addr); + } +} #endif /* _INTERNAL_H */ diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c index 5a0794cb8815..ded69108db0d 100644 --- a/security/keys/keyctl.c +++ b/security/keys/keyctl.c @@ -339,7 +339,7 @@ long keyctl_update_key(key_serial_t id, payload = NULL; if (plen) { ret = -ENOMEM; - payload = kmalloc(plen, GFP_KERNEL); + payload = kvmalloc(plen, GFP_KERNEL); if (!payload) goto error; @@ -360,7 +360,7 @@ long keyctl_update_key(key_serial_t id, key_ref_put(key_ref); error2: - kzfree(payload); + __kvzfree(payload, plen); error: return ret; } @@ -877,13 +877,23 @@ long keyctl_read_key(key_serial_t keyid, char __user *buffer, size_t buflen) * transferring them to user buffer to avoid potential * deadlock involving page fault and mmap_sem. */ - char *key_data = kmalloc(buflen, GFP_KERNEL); + char *key_data = NULL; + size_t key_data_len = buflen; - if (!key_data) { - ret = -ENOMEM; - goto error2; + /* + * When the user-supplied key length is larger than + * PAGE_SIZE, we get the actual key length first before + * allocating a right-sized key data buffer. + */ + if (buflen <= PAGE_SIZE) { +allocbuf: + key_data = kvmalloc(key_data_len, GFP_KERNEL); + if (!key_data) { + ret = -ENOMEM; + goto error2; + } } - ret = __keyctl_read_key(key, key_data, buflen); + ret = __keyctl_read_key(key, key_data, key_data_len); /* * Read methods will just return the required length @@ -891,10 +901,23 @@ long keyctl_read_key(key_serial_t keyid, char __user *buffer, size_t buflen) * enough. */ if (ret > 0 && ret <= buflen) { + /* + * The key may change (unlikely) in between 2 + * consecutive __keyctl_read_key() calls. We will + * need to allocate a larger buffer and redo the key + * read when key_data_len < ret <= buflen. + */ + if (!key_data || unlikely(ret > key_data_len)) { + if (unlikely(key_data)) + __kvzfree(key_data, key_data_len); + key_data_len = ret; + goto allocbuf; + } + if (copy_to_user(buffer, key_data, ret)) ret = -EFAULT; } - kzfree(key_data); + __kvzfree(key_data, key_data_len); } error2: -- 2.18.1
next prev parent reply other threads:[~2020-03-20 19:19 UTC|newest] Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-03-20 19:19 [PATCH v6 0/2] KEYS: Read keys to internal buffer & then copy to userspace Waiman Long 2020-03-20 19:19 ` Waiman Long 2020-03-20 19:19 ` [PATCH v6 1/2] KEYS: Don't write out to userspace while holding key semaphore Waiman Long 2020-03-20 19:19 ` Waiman Long 2020-03-20 19:19 ` Waiman Long [this message] 2020-03-20 19:19 ` [PATCH v6 2/2] KEYS: Avoid false positive ENOMEM error on key read Waiman Long 2020-03-20 22:19 ` Jarkko Sakkinen 2020-03-20 22:19 ` Jarkko Sakkinen 2020-03-20 22:21 ` Jarkko Sakkinen 2020-03-20 22:21 ` Jarkko Sakkinen 2020-03-21 1:51 ` Waiman Long 2020-03-21 1:51 ` Waiman Long
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=20200320191903.19494-3-longman@redhat.com \ --to=longman@redhat.com \ --cc=crecklin@redhat.com \ --cc=davem@davemloft.net \ --cc=dhowells@redhat.com \ --cc=ebiggers@google.com \ --cc=jarkko.sakkinen@linux.intel.com \ --cc=jmorris@namei.org \ --cc=jsnitsel@redhat.com \ --cc=keyrings@vger.kernel.org \ --cc=kuba@kernel.org \ --cc=linux-afs@lists.infradead.org \ --cc=linux-integrity@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-security-module@vger.kernel.org \ --cc=netdev@vger.kernel.org \ --cc=roberto.sassu@huawei.com \ --cc=serge@hallyn.com \ --cc=sumit.garg@linaro.org \ --cc=zohar@linux.ibm.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: linkBe 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.