From patchwork Fri May 1 13:21:12 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Greg Kroah-Hartman X-Patchwork-Id: 226666 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-9.8 required=3.0 tests=DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH, MAILING_LIST_MULTI, SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS,USER_AGENT_GIT autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id CD463C47253 for ; Fri, 1 May 2020 13:32:46 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id AA0CD208DB for ; Fri, 1 May 2020 13:32:46 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1588339966; bh=XIFEwbMmB5NjCjpQoYIdtkuwlS1Iy2GE55PxfoCqRqA=; h=From:To:Cc:Subject:Date:In-Reply-To:References:List-ID:From; b=Jo4f+TNwLN7ORp+YThgLdDkBJCGVBu8+lfCZ6iV0L3PaJ4fZF0FJD8LtxVCpEfzEo tSoLKD2ersSuEWqr3a1B22SG3E97RZzLkRRLVefS0eoHt4YfAlCw1WyQpeyjCFIxjT xtjKRbv19G6Fy8iDTWKo8X2f6edcTlzj9WoIkiAk= Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1729762AbgEANcp (ORCPT ); Fri, 1 May 2020 09:32:45 -0400 Received: from mail.kernel.org ([198.145.29.99]:57658 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1730327AbgEANcm (ORCPT ); Fri, 1 May 2020 09:32:42 -0400 Received: from localhost (83-86-89-107.cable.dynamic.v4.ziggo.nl [83.86.89.107]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id A122E2173E; Fri, 1 May 2020 13:32:39 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1588339960; bh=XIFEwbMmB5NjCjpQoYIdtkuwlS1Iy2GE55PxfoCqRqA=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=FVc+XYbgS7cNKqR/HpUPWbQps2fGiA0Kj7fGkxPHNkvBDtLC6wNID8SGzp4E4kOBv TM4iTPEfxceI5C8lXVcULWaDJM58JLZ0NDCR7x9DDlVUKzqVbp5eEBNrsRnvpzwCTW U25rhXvaaBHbIFPSz0aIZ86Rju3D3MIRgCF+OMng= From: Greg Kroah-Hartman To: linux-kernel@vger.kernel.org Cc: Greg Kroah-Hartman , stable@vger.kernel.org, Waiman Long , David Howells , Sasha Levin Subject: [PATCH 4.14 036/117] KEYS: Avoid false positive ENOMEM error on key read Date: Fri, 1 May 2020 15:21:12 +0200 Message-Id: <20200501131549.063403306@linuxfoundation.org> X-Mailer: git-send-email 2.26.2 In-Reply-To: <20200501131544.291247695@linuxfoundation.org> References: <20200501131544.291247695@linuxfoundation.org> User-Agent: quilt/0.66 MIME-Version: 1.0 Sender: stable-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: stable@vger.kernel.org From: Waiman Long [ Upstream commit 4f0882491a148059a52480e753b7f07fc550e188 ] 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. In the extremely unlikely scenario that the key keeps on being changed and made longer (still <= buflen) in between 2 __keyctl_read_key() calls, the __keyctl_read_key() calling loop in keyctl_read_key() may have to be iterated a large number of times, but definitely not infinite. Signed-off-by: Waiman Long Signed-off-by: David Howells Signed-off-by: Sasha Levin --- security/keys/internal.h | 12 +++++++++ security/keys/keyctl.c | 58 +++++++++++++++++++++++++++++----------- 2 files changed, 55 insertions(+), 15 deletions(-) diff --git a/security/keys/internal.h b/security/keys/internal.h index e3a5738401866..124273e500cfa 100644 --- a/security/keys/internal.h +++ b/security/keys/internal.h @@ -20,6 +20,8 @@ #include #include #include +#include +#include struct iovec; @@ -305,4 +307,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 4b6a084e323b5..c07c2e2b24783 100644 --- a/security/keys/keyctl.c +++ b/security/keys/keyctl.c @@ -330,7 +330,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; @@ -351,7 +351,7 @@ long keyctl_update_key(key_serial_t id, key_ref_put(key_ref); error2: - kzfree(payload); + __kvzfree(payload, plen); error: return ret; } @@ -772,7 +772,8 @@ long keyctl_read_key(key_serial_t keyid, char __user *buffer, size_t buflen) struct key *key; key_ref_t key_ref; long ret; - char *key_data; + char *key_data = NULL; + size_t key_data_len; /* find the key first */ key_ref = lookup_user_key(keyid, 0, 0); @@ -823,24 +824,51 @@ can_read_key: * Allocating a temporary buffer to hold the keys before * transferring them to user buffer to avoid potential * deadlock involving page fault and mmap_sem. + * + * key_data_len = (buflen <= PAGE_SIZE) + * ? buflen : actual length of key data + * + * This prevents allocating arbitrary large buffer which can + * be much larger than the actual key length. In the latter case, + * at least 2 passes of this loop is required. */ - key_data = kmalloc(buflen, GFP_KERNEL); + key_data_len = (buflen <= PAGE_SIZE) ? buflen : 0; + for (;;) { + if (key_data_len) { + key_data = kvmalloc(key_data_len, GFP_KERNEL); + if (!key_data) { + ret = -ENOMEM; + goto key_put_out; + } + } - if (!key_data) { - ret = -ENOMEM; - goto key_put_out; - } - 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 without + * any copying if the provided length isn't large enough. + */ + if (ret <= 0 || ret > buflen) + break; + + /* + * The key may change (unlikely) in between 2 consecutive + * __keyctl_read_key() calls. In this case, we reallocate + * a larger buffer and redo the key read when + * key_data_len < ret <= buflen. + */ + if (ret > key_data_len) { + if (unlikely(key_data)) + __kvzfree(key_data, key_data_len); + key_data_len = ret; + continue; /* Allocate buffer */ + } - /* - * Read methods will just return the required length without - * any copying if the provided length isn't large enough. - */ - if (ret > 0 && ret <= buflen) { if (copy_to_user(buffer, key_data, ret)) ret = -EFAULT; + break; } - kzfree(key_data); + __kvzfree(key_data, key_data_len); key_put_out: key_put(key);