Received-SPF: pass (google.com: domain of linux-kernel+bounces-102738-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Wed, 13 Mar 2024 21:05:55 -0600
From: "Jason A. Donenfeld" <Jason@zx2c4.com>
To: Michael Kelley <mhklinux@outlook.com>
Cc: "haiyangz@microsoft.com" <haiyangz@microsoft.com>,
	"wei.liu@kernel.org" <wei.liu@kernel.org>,
	"decui@microsoft.com" <decui@microsoft.com>,
	"tglx@linutronix.de" <tglx@linutronix.de>,
	"mingo@redhat.com" <mingo@redhat.com>,
	"bp@alien8.de" <bp@alien8.de>,
	"dave.hansen@linux.intel.com" <dave.hansen@linux.intel.com>,
	"hpa@zytor.com" <hpa@zytor.com>, "arnd@arndb.de" <arnd@arndb.de>,
	"tytso@mit.edu" <tytso@mit.edu>, "x86@kernel.org" <x86@kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"linux-hyperv@vger.kernel.org" <linux-hyperv@vger.kernel.org>,
	"linux-arch@vger.kernel.org" <linux-arch@vger.kernel.org>
Subject: Re: [PATCH v2 1/1] x86/hyperv: Use Hyper-V entropy to seed guest
 random number generator
Message-ID: <ZfJpk94mtwzRaJzv@zx2c4.com>
References: <20240307184820.70589-1-mhklinux@outlook.com>
 <ZfI3owmQOKc4Ta_X@zx2c4.com>
 <SN6PR02MB41576DD458EB3C72F3784EDBD4292@SN6PR02MB4157.namprd02.prod.outlook.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <SN6PR02MB41576DD458EB3C72F3784EDBD4292@SN6PR02MB4157.namprd02.prod.outlook.com>

Hi Michael,

On Thu, Mar 14, 2024 at 12:30:06AM +0000, Michael Kelley wrote:
> OK, fair enough.  But just to double-check:  When this is called,
> the EFI RNG protocol has already invoked add_bootloader_randomness(),
>  and this line has been output:
> 
> [    0.000000] random: crng init done
> 
> I don't see an obvious problem with calling add_bootloader_randomness()
> again, but wanted to confirm.

Yea, that's very much okay. It'll just get added as extra, which can't
hurt.

> Also, if we're adding this ACPI-based randomness for VMs that
> boot via EFI, then for consistency we should use it on Hyper-V
> based ARM64 VMs as well.

Agreed.

> Both bounds are just a check for bogusness.  Having the hypervisor
> provide just 4 bytes (for example) of randomness seems like
> there might be something weird going on.  But widening the bounds
> is fine with me.  I'll use "8" and "SZ_4K".

Ahh, as a sanity check that seems like a reasonable heuristic.

> > > +	for (i = 0; i < length; i++) {
> > > +		header->checksum += randomdata[i];
> > > +		randomdata[i] = 0;
> > > +	}
> > 
> > Seems dangerous for kexec and such. What if, in addition to zeroing out
> > the actual data, you also set header->length to 0, so that it doesn't
> > get used again as 32 bytes of known zeros?
> 
> What's your take on the whole idea of zero'ing the random data?   I
> saw the EFI RNG protocol handling was doing something roughly
> similiar.  But yes, good point about kexec().  Zeroing the header->length
> would make sense to prevent any re-use.

Yes, I think zeroing it out is the right call. I wonder, though, what's
the deal with the checksum adjustment? Should we be checking the
checksum before using the random data? And do we have to adjust it like
that at the end, or can we just zero out the entire header (including
length) along with the random data?

Jason