| Message ID | 20200318221457.1330-3-longman@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KEYS: Read keys to internal buffer & then copy to userspace | expand |
On Wed, Mar 18, 2020 at 06:14:57PM -0400, Waiman Long wrote: > + * It is possible, though unlikely, that the key > + * changes in between the up_read->down_read period. > + * If the key becomes longer, we will have to > + * allocate a larger buffer and redo the key read > + * again. > + */ > + if (!tmpbuf || unlikely(ret > tmpbuflen)) { Shouldn't you check that tmpbuflen stays below buflen (why else you had made copy of buflen otherwise)? /Jarkko
On 3/19/20 3:46 PM, Jarkko Sakkinen wrote: > On Wed, Mar 18, 2020 at 06:14:57PM -0400, Waiman Long wrote: >> + * It is possible, though unlikely, that the key >> + * changes in between the up_read->down_read period. >> + * If the key becomes longer, we will have to >> + * allocate a larger buffer and redo the key read >> + * again. >> + */ >> + if (!tmpbuf || unlikely(ret > tmpbuflen)) { > Shouldn't you check that tmpbuflen stays below buflen (why else > you had made copy of buflen otherwise)? The check above this thunk: if ((ret > 0) && (ret <= buflen)) { will make sure that ret will not be larger than buflen. So tmpbuflen will never be bigger than buflen. Cheers, Longman
On Thu, Mar 19, 2020 at 08:07:55PM -0400, Waiman Long wrote: > On 3/19/20 3:46 PM, Jarkko Sakkinen wrote: > > On Wed, Mar 18, 2020 at 06:14:57PM -0400, Waiman Long wrote: > >> + * It is possible, though unlikely, that the key > >> + * changes in between the up_read->down_read period. > >> + * If the key becomes longer, we will have to > >> + * allocate a larger buffer and redo the key read > >> + * again. > >> + */ > >> + if (!tmpbuf || unlikely(ret > tmpbuflen)) { > > Shouldn't you check that tmpbuflen stays below buflen (why else > > you had made copy of buflen otherwise)? > > The check above this thunk: > > if ((ret > 0) && (ret <= buflen)) { > > will make sure that ret will not be larger than buflen. So tmpbuflen > will never be bigger than buflen. Ah right, of course, thanks. What would go wrong if the condition was instead ((ret > 0) && (ret <= tmpbuflen))? /Jarkko
On 3/19/20 10:07 PM, Jarkko Sakkinen wrote: > On Thu, Mar 19, 2020 at 08:07:55PM -0400, Waiman Long wrote: >> On 3/19/20 3:46 PM, Jarkko Sakkinen wrote: >>> On Wed, Mar 18, 2020 at 06:14:57PM -0400, Waiman Long wrote: >>>> + * It is possible, though unlikely, that the key >>>> + * changes in between the up_read->down_read period. >>>> + * If the key becomes longer, we will have to >>>> + * allocate a larger buffer and redo the key read >>>> + * again. >>>> + */ >>>> + if (!tmpbuf || unlikely(ret > tmpbuflen)) { >>> Shouldn't you check that tmpbuflen stays below buflen (why else >>> you had made copy of buflen otherwise)? >> The check above this thunk: >> >> if ((ret > 0) && (ret <= buflen)) { >> >> will make sure that ret will not be larger than buflen. So tmpbuflen >> will never be bigger than buflen. > Ah right, of course, thanks. > > What would go wrong if the condition was instead > ((ret > 0) && (ret <= tmpbuflen))? That if statement is a check to see if the actual key length is longer than the user-supplied buffer (buflen). If that is the case, it will just return the expected length without storing anything into the user buffer. For the case that buflen >= ret > tmpbuflen, the revised check above will incorrectly skip the storing step causing the caller to incorrectly think the key is there in the buffer. Maybe I should clarify that a bit more in the comment. Cheers, Longman
On Fri, Mar 20, 2020 at 09:27:03AM -0400, Waiman Long wrote: > On 3/19/20 10:07 PM, Jarkko Sakkinen wrote: > > On Thu, Mar 19, 2020 at 08:07:55PM -0400, Waiman Long wrote: > >> On 3/19/20 3:46 PM, Jarkko Sakkinen wrote: > >>> On Wed, Mar 18, 2020 at 06:14:57PM -0400, Waiman Long wrote: > >>>> + * It is possible, though unlikely, that the key > >>>> + * changes in between the up_read->down_read period. > >>>> + * If the key becomes longer, we will have to > >>>> + * allocate a larger buffer and redo the key read > >>>> + * again. > >>>> + */ > >>>> + if (!tmpbuf || unlikely(ret > tmpbuflen)) { > >>> Shouldn't you check that tmpbuflen stays below buflen (why else > >>> you had made copy of buflen otherwise)? > >> The check above this thunk: > >> > >> if ((ret > 0) && (ret <= buflen)) { > >> > >> will make sure that ret will not be larger than buflen. So tmpbuflen > >> will never be bigger than buflen. > > Ah right, of course, thanks. > > > > What would go wrong if the condition was instead > > ((ret > 0) && (ret <= tmpbuflen))? > > That if statement is a check to see if the actual key length is longer > than the user-supplied buffer (buflen). If that is the case, it will > just return the expected length without storing anything into the user > buffer. For the case that buflen >= ret > tmpbuflen, the revised check > above will incorrectly skip the storing step causing the caller to > incorrectly think the key is there in the buffer. > > Maybe I should clarify that a bit more in the comment. OK, right because it is possible in-between tmpbuflen could be larger. Got it. I think that longish key_data and key_data_len would be better names than tmpbuf and tpmbuflen. Also the comments are somewat overkill IMHO. I'd replace them along the lines of /* Cap the user supplied buffer length to PAGE_SIZE. */ /* Key data can change as we don not hold key->sem. */ /Jarkko
On 3/20/20 10:35 AM, Jarkko Sakkinen wrote: > On Fri, Mar 20, 2020 at 09:27:03AM -0400, Waiman Long wrote: >> On 3/19/20 10:07 PM, Jarkko Sakkinen wrote: >>> On Thu, Mar 19, 2020 at 08:07:55PM -0400, Waiman Long wrote: >>>> On 3/19/20 3:46 PM, Jarkko Sakkinen wrote: >>>>> On Wed, Mar 18, 2020 at 06:14:57PM -0400, Waiman Long wrote: >>>>>> + * It is possible, though unlikely, that the key >>>>>> + * changes in between the up_read->down_read period. >>>>>> + * If the key becomes longer, we will have to >>>>>> + * allocate a larger buffer and redo the key read >>>>>> + * again. >>>>>> + */ >>>>>> + if (!tmpbuf || unlikely(ret > tmpbuflen)) { >>>>> Shouldn't you check that tmpbuflen stays below buflen (why else >>>>> you had made copy of buflen otherwise)? >>>> The check above this thunk: >>>> >>>> if ((ret > 0) && (ret <= buflen)) { >>>> >>>> will make sure that ret will not be larger than buflen. So tmpbuflen > >> will never be bigger than buflen. > > Ah right, of course, thanks. >>> What would go wrong if the condition was instead >>> ((ret > 0) && (ret <= tmpbuflen))? >> That if statement is a check to see if the actual key length is longer >> than the user-supplied buffer (buflen). If that is the case, it will >> just return the expected length without storing anything into the user >> buffer. For the case that buflen >= ret > tmpbuflen, the revised check >> above will incorrectly skip the storing step causing the caller to >> incorrectly think the key is there in the buffer. >> >> Maybe I should clarify that a bit more in the comment. > OK, right because it is possible in-between tmpbuflen could be > larger. Got it. > > I think that longish key_data and key_data_len would be better > names than tmpbuf and tpmbuflen. > > Also the comments are somewat overkill IMHO. > > I'd replace them along the lines of > > /* Cap the user supplied buffer length to PAGE_SIZE. */ > > /* Key data can change as we don not hold key->sem. */ I am fine with the rename, will sent out a v6 soon. Cheers, Longman
Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> wrote:
> /* Key data can change as we don not hold key->sem. */
I think you mean "we don't".
David
On Fri, Mar 20, 2020 at 11:55:10PM +0000, David Howells wrote: > Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> wrote: > > > /* Key data can change as we don not hold key->sem. */ > > I think you mean "we don't". Oops, typo. Yes, thanks. /Jarkko
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 81f68e434b9f..07eaa46d344c 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,24 @@ 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 *tmpbuf = kmalloc(buflen, GFP_KERNEL); + char *tmpbuf = NULL; + size_t tmpbuflen = buflen; - if (!tmpbuf) { - ret = -ENOMEM; - goto error2; + /* + * To prevent memory allocation failure with an arbitrary + * large user-supplied buflen, we do a key length check + * before allocating a buffer of the right size to hold + * key data if it exceeds a threshold (PAGE_SIZE). + */ + if (buflen <= PAGE_SIZE) { +allocbuf: + tmpbuf = kvmalloc(tmpbuflen, GFP_KERNEL); + if (!tmpbuf) { + ret = -ENOMEM; + goto error2; + } } - ret = __keyctl_read_key(key, tmpbuf, buflen); + ret = __keyctl_read_key(key, tmpbuf, tmpbuflen); /* * Read methods will just return the required length @@ -891,10 +902,24 @@ long keyctl_read_key(key_serial_t keyid, char __user *buffer, size_t buflen) * enough. */ if ((ret > 0) && (ret <= buflen)) { + /* + * It is possible, though unlikely, that the key + * changes in between the up_read->down_read period. + * If the key becomes longer, we will have to + * allocate a larger buffer and redo the key read + * again. + */ + if (!tmpbuf || unlikely(ret > tmpbuflen)) { + if (unlikely(tmpbuf)) + __kvzfree(tmpbuf, tmpbuflen); + tmpbuflen = ret; + goto allocbuf; + } + if (copy_to_user(buffer, tmpbuf, ret)) ret = -EFAULT; } - kzfree(tmpbuf); + __kvzfree(tmpbuf, tmpbuflen); } error2:
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 | 41 ++++++++++++++++++++++++++++++++-------- 2 files changed, 45 insertions(+), 8 deletions(-)