Received-SPF: pass (google.com: domain of linux-kernel+bounces-103824-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Message-ID: <8f6d7c14-5341-4d13-9538-d34a18d3c117@intel.com>
Date: Fri, 15 Mar 2024 11:07:18 +1300
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v2] KVM: x86/mmu: x86: Don't overflow lpage_info when
 checking attributes
Content-Language: en-US
To: Rick Edgecombe <rick.p.edgecombe@intel.com>, <seanjc@google.com>,
	<pbonzini@redhat.com>, <chao.p.peng@linux.intel.com>,
	<isaku.yamahata@intel.com>
CC: <kvm@vger.kernel.org>, <linux-kernel@vger.kernel.org>
References: <20240314212902.2762507-1-rick.p.edgecombe@intel.com>
From: "Huang, Kai" <kai.huang@intel.com>
In-Reply-To: <20240314212902.2762507-1-rick.p.edgecombe@intel.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?OXF5RWF6S2ZjRWtnUklwNW1vRmw5YjUzM1dka0NjdEYveXBYZmJwSGRqNTda?=
 =?utf-8?B?cSthMEc5T1F5MkFhTnZ2cUVzVkwwOUF5QVpEUWNRNXhES1ZuTzVZeE93ektS?=
 =?utf-8?B?Q0dRek1UY3JyZFloSGhZQXBxTFY2VmticHZIMnZqZVRJUUkvbkFFUTJGejYv?=
 =?utf-8?B?ZkdQQ21nbTNLOEpTMkxrZlJ6cE9CZ2FFdGsyZEptZkFzbUhRa2FlUVpnclM2?=
 =?utf-8?B?VytVZWR6WXdaRjNkd1RtbE1Takk0OG5BeUhQK2xyenlTaVJzcmtYNHpZeFY0?=
 =?utf-8?B?bDhaNGFxT3F5bUtmcnZYZnYzOVFaVTIrR1Z2QzVBZDhYRmtwZGdSTHg0NnVD?=
 =?utf-8?B?Y3kvakxOYXZ3UzRTYTV5WTNhOEV6VnJQMW1Dd1RVREZCQ3dIR1VXLzdWRDNr?=
 =?utf-8?B?Y0ZQNzBHMER3SnBaMXM3bWdSdG1DaEZLWTVHbWFQa1Jub2p1WTkzcm10Ykxv?=
 =?utf-8?B?Qm1JR3krUEI3TG1xenZvcTZ4WUVCTDRxMEhiUU1kdkRtNkhCWWxrcUtUckJ3?=
 =?utf-8?B?YUwyVDh0S09oK2ZHN2JrN0Y3OGpwSUVBNy8xSWZBemZnQjBhL0pXampoTGl3?=
 =?utf-8?B?WDFmeE1qRmNobUhxMHhWT29OSHYyK0dEc2xpT3VUU25kWHhETVREL2p0OVRZ?=
 =?utf-8?B?Z0RHNElnT2hUZlFwcHlpYTZEMW5OS0MvNHVGMTZkTWFobDdQZzd2ZFMwVW5s?=
 =?utf-8?B?RWFISVFtRU9sRTF5dlFrbWNZdGJRVnRTdGRDVkdqODhLVGJOOGVZdmhQbHF0?=
 =?utf-8?B?QTdvUStoVWxoSTdCczRXcklTVEJzbnpZTUx1bG5QOEJiVkZhSjUxbXNBR0U5?=
 =?utf-8?B?eGt2NVdhTzNZYlo1L01oeUlTbUhLM1FsSE1LM2pwclN1SVovWmg3L0tSZzdJ?=
 =?utf-8?B?dWxvNGdxNitiUW15N1Nta0JXall1aGViYW5UbHNINTh5bXVjeGhJc2Y4QmJ1?=
 =?utf-8?B?anowQlFjYnFWQmszY0ROV29BS0ZNMHJiRWxkUEV5Z3c1SlNxYS8wUXovbEdK?=
 =?utf-8?B?Q1Aza0ZGbzZmSE9ZeFRYenVDQ3VtRXZYdTVsNlc5YkpIU3R6NzZkRGFrS1dJ?=
 =?utf-8?B?anZxMmtCZkVFUjhsSE5lY3o4QkFaSUliQVA0R2hyWUcwZkRscXc2c0xRazFj?=
 =?utf-8?B?OWNDN0JmcXVOWVdObDc3NFNaQUkxY2NiNUl1bmJyZ3pQZU5ZVzZOV0loWG1Z?=
 =?utf-8?B?bDJ6Y0lEN0FocEl0dmZNVm1POFlYb1hTY3NKL3pnUUJnSG9DdGVSeS9iQlFE?=
 =?utf-8?B?QnhCbUVjNzlXaVUwUWpHN2o5SjUzWGdVUlZJR0ZqVnR3WG5CTWMvMndEYUcz?=
 =?utf-8?B?OXFJemIvQ3lBaXF0UUFkZUY1SzMxd0xDcUdNQUZSZmdGaklTSUY0T1Z3UWh1?=
 =?utf-8?B?QjhEejllZTEzdThBV2oyZk44RjRHeGJBVktZT3RGQkY5TmNMVDUxZjRIUG1w?=
 =?utf-8?B?akYyWW5zaktsVnlXSEF1RXBndE1Yb0had2c4dUhjRVRSYWNrOEMyeUVqd050?=
 =?utf-8?B?ZDBWOEtDZ0s0b1hwZ2daQmJvWWdLanpIdlptc3VQMW9URnYxdC9lWHVpNjZt?=
 =?utf-8?B?TXE2eGtCQTEvbVZ5ZktOTXFTSGdqVlVpSlNTVE15SEFBelg3V0VoTmZrWFVM?=
 =?utf-8?B?T1FsYXNBbGpIdGlKT2RQTDhEalM1SzB6V2o2Q3l5YjB5V1FiTHR1Y2MrTnB6?=
 =?utf-8?B?VThxeGIzOXZOUUh4OTU3ZUExSnk2eXVBN2xTbWZYT05HUmJEdE12ci9hVXpo?=
 =?utf-8?B?cTVsdWJrUnQ1Q1FOSUVkcHU4SENKTTQwQ1B4MVoxdXBsZ0Nmc3FZMlNubnhD?=
 =?utf-8?B?ajBFTEhxczJZdXlPTkJoZzRKL0VoLzhNNlZkMDQvSFN0Vk0yOVNtb0JtYmZS?=
 =?utf-8?B?V2k2K0x1N2ZFYWRHdzh3T1RDY0UyU3BLaGJzWGZtY2RuMWx6S1NUQm5LeU1j?=
 =?utf-8?B?Q00xK2cwQkxzVmpkZnorM002YjY4aThrWjNGeWVOWlhHeWhHQTl2RkJ5VjFF?=
 =?utf-8?B?dEZXVTdISTErdG1od2ZmZnJJbXVKRGdZM2pRWXpLd3lqUWkxZ0hyUUdwcjBQ?=
 =?utf-8?B?YnpGZyt1MDZ5RDZUSEdzWlV4RUcvQ05GcGw5SzhOYjNDQ0dkUmZKK3JwOG5v?=
 =?utf-8?Q?buEqTBk9yQwQBw1mXNAmay2Gr?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 9588ce74-dd37-4783-42f0-08dc4473224e
X-MS-Exchange-CrossTenant-AuthSource: BL1PR11MB5978.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 14 Mar 2024 22:07:26.9035
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: 2mcApI6P4u+hg16xpspfSKE+pPWfKeyZc6ua4rElXT2WQZCHyAGxyJeTCPppLVqLSZLspkCUe+wMgyGJnyaH4A==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH3PR11MB7722
X-OriginatorOrg: intel.com


On 15/03/2024 10:29 am, Rick Edgecombe wrote:
> Fix KVM_SET_MEMORY_ATTRIBUTES to not overflow lpage_info array and trigger
> KASAN splat, as seen in the private_mem_conversions_test selftest.
> 
> When memory attributes are set on a GFN range, that range will have
> specific properties applied to the TDP. A huge page cannot be used when
> the attributes are inconsistent, so they are disabled for those the
> specific huge pages. For internal KVM reasons, huge pages are also not
> allowed to span adjacent memslots regardless of whether the backing memory
> could be mapped as huge.
> 
> What GFNs support which huge page sizes is tracked by an array of arrays
> 'lpage_info' on the memslot, of ‘kvm_lpage_info’ structs. Each index of
> lpage_info contains a vmalloc allocated array of these for a specific
> supported page size. The kvm_lpage_info denotes whether a specific huge
> page (GFN and page size) on the memslot is supported. These arrays include
> indices for unaligned head and tail huge pages.
> 
> Preventing huge pages from spanning adjacent memslot is covered by
> incrementing the count in head and tail kvm_lpage_info when the memslot is
> allocated, but disallowing huge pages for memory that has mixed attributes
> has to be done in a more complicated way. During the
> KVM_SET_MEMORY_ATTRIBUTES ioctl KVM updates lpage_info for each memslot in
> the range that has mismatched attributes. KVM does this a memslot at a
> time, and marks a special bit, KVM_LPAGE_MIXED_FLAG, in the kvm_lpage_info
> for any huge page. This bit is essentially a permanently elevated count.
> So huge pages will not be mapped for the GFN at that page size if the
> count is elevated in either case: a huge head or tail page unaligned to
> the memslot or if KVM_LPAGE_MIXED_FLAG is set because it has mixed
> attributes.
> 
> To determine whether a huge page has consistent attributes, the
> KVM_SET_MEMORY_ATTRIBUTES operation checks an xarray to make sure it
> consistently has the incoming attribute. Since level - 1 huge pages are
> aligned to level huge pages, it employs an optimization. As long as the
> level - 1 huge pages are checked first, it can just check these and assume
> that if each level - 1 huge page contained within the level sized huge
> page is not mixed, then the level size huge page is not mixed. This
> optimization happens in the helper hugepage_has_attrs().
> 
> Unfortunately, although the kvm_lpage_info array representing page size
> 'level' will contain an entry for an unaligned tail page of size level,
> the array for level - 1  will not contain an entry for each GFN at page
> size level. The level - 1 array will only contain an index for any
> unaligned region covered by level - 1 huge page size, which can be a
> smaller region. So this causes the optimization to overflow the level - 1
> kvm_lpage_info and perform a vmalloc out of bounds read.
> 
> In some cases of head and tail pages where an overflow could happen,
> callers skip the operation completely as KVM_LPAGE_MIXED_FLAG is not
> required to prevent huge pages as discussed earlier. But for memslots that
> are smaller than the 1GB page size, it does call hugepage_has_attrs(). In
> this case the huge page is both the head and tail page. The issue can be
> observed simply by compiling the kernel with CONFIG_KASAN_VMALLOC and
> running the selftest “private_mem_conversions_test”, which produces the
> output like the following:
> 
> BUG: KASAN: vmalloc-out-of-bounds in hugepage_has_attrs+0x7e/0x110
> Read of size 4 at addr ffffc900000a3008 by task private_mem_con/169
> Call Trace:
>    dump_stack_lvl
>    print_report
>    ? __virt_addr_valid
>    ? hugepage_has_attrs
>    ? hugepage_has_attrs
>    kasan_report
>    ? hugepage_has_attrs
>    hugepage_has_attrs
>    kvm_arch_post_set_memory_attributes
>    kvm_vm_ioctl
> 
> It is a little ambiguous whether the unaligned head page (in the bug case
> also the tail page) should be expected to have KVM_LPAGE_MIXED_FLAG set.
> It is not functionally required, as the unaligned head/tail pages will
> already have their kvm_lpage_info count incremented. The comments imply
> not setting it on unaligned head pages is intentional, so fix the callers
> to skip trying to set KVM_LPAGE_MIXED_FLAG in this case, and in doing so
> not call hugepage_has_attrs().
> 
> Cc: stable@vger.kernel.org
> Fixes: 90b4fe17981e ("KVM: x86: Disallow hugepages when memory attributes are mixed")
> Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>

Reviewed-by: Kai Huang <kai.huang@intel.com>

> ---
> v2:
>   - Drop function rename (Sean)
>   - Clarify in commit log that this is only head pages that are also tail
>     pages (Sean)
> ---
>   arch/x86/kvm/mmu/mmu.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
> index 0544700ca50b..42e7de604bb6 100644
> --- a/arch/x86/kvm/mmu/mmu.c
> +++ b/arch/x86/kvm/mmu/mmu.c
> @@ -7388,7 +7388,8 @@ bool kvm_arch_post_set_memory_attributes(struct kvm *kvm,
>   			 * by the memslot, KVM can't use a hugepage due to the
>   			 * misaligned address regardless of memory attributes.
>   			 */
> -			if (gfn >= slot->base_gfn) {
> +			if (gfn >= slot->base_gfn &&
> +			    gfn + nr_pages <= slot->base_gfn + slot->npages) {
>   				if (hugepage_has_attrs(kvm, slot, gfn, level, attrs))
>   					hugepage_clear_mixed(slot, gfn, level);
>   				else