| 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:
<SN6PR02MB41571826E243F10A2624FD81D4EE2@SN6PR02MB4157.namprd02.prod.outlook.com>
Date: Wed, 29 Jan 2025 01:30:24 +0000
From: Michael Kelley <mhklinux@...look.com>
To: Nuno Das Neves <nunodasneves@...ux.microsoft.com>,
"linux-hyperv@...r.kernel.org" <linux-hyperv@...r.kernel.org>,
"linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>, "linux-kernel@...r.kernel.org"
<linux-kernel@...r.kernel.org>, "linux-arch@...r.kernel.org"
<linux-arch@...r.kernel.org>, "wei.liu@...nel.org" <wei.liu@...nel.org>
CC: "kys@...rosoft.com" <kys@...rosoft.com>, "haiyangz@...rosoft.com"
<haiyangz@...rosoft.com>, "decui@...rosoft.com" <decui@...rosoft.com>,
"catalin.marinas@....com" <catalin.marinas@....com>, "will@...nel.org"
<will@...nel.org>, "tglx@...utronix.de" <tglx@...utronix.de>,
"mingo@...hat.com" <mingo@...hat.com>, "bp@...en8.de" <bp@...en8.de>,
"dave.hansen@...ux.intel.com" <dave.hansen@...ux.intel.com>, "x86@...nel.org"
<x86@...nel.org>, "hpa@...or.com" <hpa@...or.com>, "arnd@...db.de"
<arnd@...db.de>, "jinankjain@...ux.microsoft.com"
<jinankjain@...ux.microsoft.com>, "muminulrussell@...il.com"
<muminulrussell@...il.com>, "skinsburskii@...ux.microsoft.com"
<skinsburskii@...ux.microsoft.com>, "mukeshrathor@...rosoft.com"
<mukeshrathor@...rosoft.com>
Subject: RE: [PATCH v2 1/2] hyperv: Move hv_current_partition_id to
arch-generic code
From: Nuno Das Neves <nunodasneves@...ux.microsoft.com> Sent: Tuesday, January 28, 2025 4:46 PM
>
> On 1/28/2025 10:45 AM, Michael Kelley wrote:
> > From: Nuno Das Neves <nunodasneves@...ux.microsoft.com> Sent: Wednesday, January 22, 2025 5:48 PM
> >>
> >> Move hv_current_partition_id and hv_get_partition_id() to hv_common.c.
> >> These aren't specific to x86_64 and will be needed by common code.
> >>
> >> Set hv_current_partition_id to HV_PARTITION_ID_SELF by default.
> >>
> >> Use a stack variable for the output of the hypercall. This allows moving
> >> the call of hv_get_partition_id() to hv_common_init() before the percpu
> >> pages are initialized.
> >>
> >> Remove the BUG()s. Failing to get the id need not crash the machine.
> >>
> >> Signed-off-by: Nuno Das Neves <nudasnev@...rosoft.com>
> >> ---
> >> arch/x86/hyperv/hv_init.c | 26 --------------------------
> >> arch/x86/include/asm/mshyperv.h | 2 --
> >> drivers/hv/hv_common.c | 23 +++++++++++++++++++++++
> >> include/asm-generic/mshyperv.h | 1 +
> >> 4 files changed, 24 insertions(+), 28 deletions(-)
[snip]
> >> diff --git a/drivers/hv/hv_common.c b/drivers/hv/hv_common.c
> >> index af5d1dc451f6..1da19b64ef16 100644
> >> --- a/drivers/hv/hv_common.c
> >> +++ b/drivers/hv/hv_common.c
> >> @@ -31,6 +31,9 @@
> >> #include <hyperv/hvhdk.h>
> >> #include <asm/mshyperv.h>
> >>
> >> +u64 hv_current_partition_id = HV_PARTITION_ID_SELF;
> >> +EXPORT_SYMBOL_GPL(hv_current_partition_id);
> >> +
> >> /*
> >> * hv_root_partition, ms_hyperv and hv_nested are defined here with other
> >> * Hyper-V specific globals so they are shared across all architectures and are
> >> @@ -283,6 +286,23 @@ static inline bool hv_output_page_exists(void)
> >> return hv_root_partition || IS_ENABLED(CONFIG_HYPERV_VTL_MODE);
> >> }
> >>
> >> +static void __init hv_get_partition_id(void)
> >> +{
> >> + /*
> >> + * Note in this case the output can be on the stack because it is just
> >> + * a single u64 and hence won't cross a page boundary.
> >> + */
> >> + struct hv_get_partition_id output;
> >
> > It's unfortunate that the structure name "hv_get_partition_id" is also
> > the name of this function. Could the structure name be changed to
> > follow the pattern of having "output" in the name, like other hypercall
> > parameters? It's not a blocker if it can't be changed. I was just surprised
> > to search for "hv_get_partition_id" and find both uses.
> >
>
> hv_output_get_partition_id is really the "correct" name from the Hyper-V code,
> so it makes sense to just change it to that in this patch.
>
> > Also, see the comment at the beginning of hv_query_ext_cap() regarding
> > using a local stack variable as hypercall input or output. The comment
> > originated here [1]. At that time, I didn't investigate Sunil's assertion any
> > further, and I'm still unsure whether it is really true. But perhaps for
> > consistency and safety we should follow what it says.
> >
> > [1] https://lore.kernel.org/linux-hyperv/SN4PR2101MB0880DB0606A5A0B72AD244B4C06A9@SN4PR2101MB0880.namprd21.prod.outlook.com/
> >
> Hmm, from some cursory research it does look like stack variables can't be
> used with virt_to_phys().
>
> I thought about just using &hv_current_partition directly - I *think* that
> will work - but in the end I think it's just simpler to just move calls so the
> percpu output page can be used as normal. That may save some additional
> back-and-forth as well as explanatory comments in the code.
>
> I will also add a check for hv_output_page_exists() here, as a precaution in
> case the HV_ACCESS_PARTITION_ID privilege ever becomes decoupled from
> that (as it stands, I believe that permission is only for the root
> partition, but you never know).
Or just use the hyperv_pcpu_input_page, even though the use here is
for output. Then you don't have to worry about whether the output
page exists. FWIW, I'm working on a set of changes that encapsulates
the assignment of the per-cpu hypercall argument pages, and it does
away with the distinction between the input and output pages. But
that will come sometime after this patch.
Michael
>
> >> + u64 status;
> >> +
> >> + status = hv_do_hypercall(HVCALL_GET_PARTITION_ID, NULL, &output);
> >> + if (!hv_result_success(status)) {
> >> + pr_err("Hyper-V: failed to get partition ID: %#lx\n", status);
> >> + return;
> >> + }
> >> + hv_current_partition_id = output.partition_id;
> >> +}
> >> +
> >> int __init hv_common_init(void)
> >> {
> >> int i;
> >> @@ -298,6 +318,9 @@ int __init hv_common_init(void)
> >> if (hv_is_isolation_supported())
> >> sysctl_record_panic_msg = 0;
> >>
> >> + if (ms_hyperv.priv_high & HV_ACCESS_PARTITION_ID)
> >> + hv_get_partition_id();
> >
> > I don't see how this works. On the x86 side, hv_common_init()
> > is called before the guest ID is set and the hypercall page is setup.
> > So the hypercall in hv_get_partition_id() should fail.
> >
>
> Oh, I tried to get too clever. I will put it back where it was and
> add it on the arm64 side to hyperv_init() after the per-cpu init as
> I mentioned above.
>
> Thanks for the comments,
> Nuno
Powered by blists - more mailing lists