| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20241104103353.GQZyijEalUocS_yG5r@fat_crate.local>
Date: Mon, 4 Nov 2024 11:33:53 +0100
From: Borislav Petkov <bp@...en8.de>
To: Tom Lendacky <thomas.lendacky@....com>
Cc: linux-kernel@...r.kernel.org, x86@...nel.org,
Thomas Gleixner <tglx@...utronix.de>,
Ingo Molnar <mingo@...hat.com>,
Dave Hansen <dave.hansen@...ux.intel.com>,
Michael Roth <michael.roth@....com>,
Ashish Kalra <ashish.kalra@....com>,
Nikunj A Dadhania <nikunj@....com>,
Neeraj Upadhyay <Neeraj.Upadhyay@....com>
Subject: Re: [PATCH v5 6/8] x86/sev: Treat the contiguous RMP table as a
single RMP segment
On Mon, Oct 28, 2024 at 02:32:41PM -0500, Tom Lendacky wrote:
> diff --git a/arch/x86/virt/svm/sev.c b/arch/x86/virt/svm/sev.c
> index 5871c924c0b2..37ff4f98e8d1 100644
> --- a/arch/x86/virt/svm/sev.c
> +++ b/arch/x86/virt/svm/sev.c
> @@ -18,6 +18,7 @@
> #include <linux/cpumask.h>
> #include <linux/iommu.h>
> #include <linux/amd-iommu.h>
> +#include <linux/nospec.h>
What's that include for?
For array_index_nospec() below?
It builds without it or did you trigger a build error with some funky .config?
> #include <asm/sev.h>
> #include <asm/processor.h>
> @@ -77,12 +78,42 @@ struct rmpentry_raw {
> */
> #define RMPTABLE_CPU_BOOKKEEPING_SZ 0x4000
>
> +/*
> + * For a non-segmented RMP table, use the maximum physical addressing as the
> + * segment size in order to always arrive at index 0 in the table.
> + */
> +#define RMPTABLE_NON_SEGMENTED_SHIFT 52
> +
> +struct rmp_segment_desc {
> + struct rmpentry_raw *rmp_entry;
> + u64 max_index;
> + u64 size;
> +};
> +
> +/*
> + * Segmented RMP Table support.
> + * - The segment size is used for two purposes:
> + * - Identify the amount of memory covered by an RMP segment
> + * - Quickly locate an RMP segment table entry for a physical address
> + *
> + * - The RMP segment table contains pointers to an RMP table that covers
> + * a specific portion of memory. There can be up to 512 8-byte entries,
> + * one pages worth.
> + */
> +static struct rmp_segment_desc **rmp_segment_table __ro_after_init;
> +static unsigned int rst_max_index __ro_after_init = 512;
> +
> +static u64 rmp_segment_size_max;
This one is used in a single function. Generate it there pls.
> +static unsigned int rmp_segment_coverage_shift;
> +static u64 rmp_segment_coverage_size;
> +static u64 rmp_segment_coverage_mask;
That "_coverage_" in there looks redundant to me and could go and make those
variables shorter.
> +#define RST_ENTRY_INDEX(x) ((x) >> rmp_segment_coverage_shift)
> +#define RMP_ENTRY_INDEX(x) ((u64)(PHYS_PFN((x) & rmp_segment_coverage_mask)))
> +
> /* Mask to apply to a PFN to get the first PFN of a 2MB page */
> #define PFN_PMD_MASK GENMASK_ULL(63, PMD_SHIFT - PAGE_SHIFT)
>
> static u64 probed_rmp_base, probed_rmp_size;
> -static struct rmpentry_raw *rmptable __ro_after_init;
> -static u64 rmptable_max_pfn __ro_after_init;
>
> static LIST_HEAD(snp_leaked_pages_list);
> static DEFINE_SPINLOCK(snp_leaked_pages_list_lock);
> @@ -190,6 +221,92 @@ static bool __init init_rmptable_bookkeeping(void)
> return true;
> }
>
> +static bool __init alloc_rmp_segment_desc(u64 segment_pa, u64 segment_size, u64 pa)
> +{
> + struct rmp_segment_desc *desc;
> + void *rmp_segment;
> + u64 rst_index;
> +
> + /* Validate the RMP segment size */
> + if (segment_size > rmp_segment_size_max) {
> + pr_err("Invalid RMP size (%#llx) for configured segment size (%#llx)\n",
> + segment_size, rmp_segment_size_max);
> + return false;
> + }
> +
> + /* Validate the RMP segment table index */
> + rst_index = RST_ENTRY_INDEX(pa);
> + if (rst_index >= rst_max_index) {
> + pr_err("Invalid RMP segment base address (%#llx) for configured segment size (%#llx)\n",
> + pa, rmp_segment_coverage_size);
> + return false;
> + }
> + rst_index = array_index_nospec(rst_index, rst_max_index);
Why are we doing this here? Are you expecting some out-of-bounds
user-controlled values here?
AFAICT, this is all read from the hw/fw so why are speculative accesses
a problem?
> + if (rmp_segment_table[rst_index]) {
> + pr_err("RMP segment descriptor already exists at index %llu\n", rst_index);
> + return false;
> + }
> +
> + /* Map the RMP entries */
Kinda obvious...
> + rmp_segment = memremap(segment_pa, segment_size, MEMREMAP_WB);
> + if (!rmp_segment) {
> + pr_err("Failed to map RMP segment addr 0x%llx size 0x%llx\n",
> + segment_pa, segment_size);
> + return false;
> + }
> +
> + desc = kzalloc(sizeof(*desc), GFP_KERNEL);
sizeof(struct rmp_segment_desc)
> + if (!desc) {
> + memunmap(rmp_segment);
> + return false;
> + }
> +
> + desc->rmp_entry = rmp_segment;
> + desc->max_index = segment_size / sizeof(*desc->rmp_entry);
> + desc->size = segment_size;
> +
> + /* Add the segment descriptor to the table */
> + rmp_segment_table[rst_index] = desc;
> +
> + return true;
> +}
...
> @@ -248,12 +366,20 @@ static int __init snp_rmptable_init(void)
>
> /* Zero out the RMP bookkeeping area */
> if (!init_rmptable_bookkeeping()) {
> - memunmap(rmptable_start);
> + free_rmp_segment_table();
> goto nosnp;
> }
>
> /* Zero out the RMP entries */
> - memset(rmptable_start, 0, rmptable_size);
> + for (i = 0; i < rst_max_index; i++) {
> + struct rmp_segment_desc *desc;
> +
> + desc = rmp_segment_table[i];
> + if (!desc)
> + continue;
> +
> + memset(desc->rmp_entry, 0, desc->size);
> + }
Why isn't this zeroing out part of alloc_rmp_segment_table()?
> @@ -319,13 +458,29 @@ bool snp_probe_rmptable_info(void)
>
> static struct rmpentry_raw *get_raw_rmpentry(u64 pfn)
> {
> - if (!rmptable)
> + struct rmp_segment_desc *desc;
> + u64 paddr, rst_index, segment_index;
Reverse xmas tree order pls.
> + if (!rmp_segment_table)
> return ERR_PTR(-ENODEV);
>
> - if (unlikely(pfn > rmptable_max_pfn))
> + paddr = pfn << PAGE_SHIFT;
> +
> + rst_index = RST_ENTRY_INDEX(paddr);
> + if (unlikely(rst_index >= rst_max_index))
> + return ERR_PTR(-EFAULT);
> + rst_index = array_index_nospec(rst_index, rst_max_index);
Same question as above.
> +
> + desc = rmp_segment_table[rst_index];
> + if (unlikely(!desc))
> return ERR_PTR(-EFAULT);
>
> - return rmptable + pfn;
> + segment_index = RMP_ENTRY_INDEX(paddr);
> + if (unlikely(segment_index >= desc->max_index))
> + return ERR_PTR(-EFAULT);
> + segment_index = array_index_nospec(segment_index, desc->max_index);
> +
> + return desc->rmp_entry + segment_index;
> }
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
Powered by blists - more mailing lists