linux-kernel - Re: [PATCH v5 4/7] xen: Put EFI machinery in place

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Mon, 16 Jun 2014 20:45:27 +0200
From:	Daniel Kiper <daniel.kiper@...cle.com>
To:	Stefano Stabellini <stefano.stabellini@...citrix.com>
Cc:	linux-efi@...r.kernel.org, linux-kernel@...r.kernel.org,
	x86@...nel.org, xen-devel@...ts.xenproject.org,
	andrew.cooper3@...rix.com, boris.ostrovsky@...cle.com,
	david.vrabel@...rix.com, eshelton@...ox.com, hpa@...or.com,
	ian.campbell@...rix.com, jbeulich@...e.com, jeremy@...p.org,
	konrad.wilk@...cle.com, matt.fleming@...el.com, mingo@...hat.com,
	mjg59@...f.ucam.org, tglx@...utronix.de
Subject: Re: [PATCH v5 4/7] xen: Put EFI machinery in place

On Mon, Jun 16, 2014 at 12:55:35PM +0100, Stefano Stabellini wrote:
> On Fri, 13 Jun 2014, Daniel Kiper wrote:
> > This patch enables EFI usage under Xen dom0. Standard EFI Linux
> > Kernel infrastructure cannot be used because it requires direct
> > access to EFI data and code. However, in dom0 case it is not possible
> > because above mentioned EFI stuff is fully owned and controlled
> > by Xen hypervisor. In this case all calls from dom0 to EFI must
> > be requested via special hypercall which in turn executes relevant
> > EFI code in behalf of dom0.
> >
> > When dom0 kernel boots it checks for EFI availability on a machine.
> > If it is detected then artificial EFI system table is filled.
> > Native EFI callas are replaced by functions which mimics them
> > by calling relevant hypercall. Later pointer to EFI system table
> > is passed to standard EFI machinery and it continues EFI subsystem
> > initialization taking into account that there is no direct access
> > to EFI boot services, runtime, tables, structures, etc. After that
> > system runs as usual.
> >
> > This patch is based on Jan Beulich and Tang Liang work.
> >
> > v5 - suggestions/fixes:
> >    - improve macro usage readability
> >      (suggested by Andrew Cooper and David Vrabel),
> >    - conditions cleanup
> >      (suggested by David Vrabel),
> >    - use -fshort-wchar option
> >      (suggested by Jan Beulich),
> >    - Kconfig rule cleanup
> >      (suggested by Jan Beulich),
> >    - forward port fixes from SUSE kernel
> >      (suggested by Jan Beulich),
> >    - improve commit message
> >      (suggested by David Vrabel).
> >
> > v4 - suggestions/fixes:
> >    - "just populate an efi_system_table_t object"
> >      (suggested by Matt Fleming).
> >
> > Signed-off-by: Jan Beulich <jbeulich@...e.com>
> > Signed-off-by: Tang Liang <liang.tang@...cle.com>
> > Signed-off-by: Daniel Kiper <daniel.kiper@...cle.com>
>
> Sorry for commenting on your patches so late in the review cycle.

No problem.

[...]

> > +efi_system_table_t __init *xen_efi_probe(void)
> > +{
> > +	struct xen_platform_op op = {
> > +		.cmd = XENPF_firmware_info,
> > +		.u.firmware_info = {
> > +			.type = XEN_FW_EFI_INFO,
> > +			.index = XEN_FW_EFI_CONFIG_TABLE
> > +		}
> > +	};
> > +	union xenpf_efi_info *info = &op.u.firmware_info.u.efi_info;
> > +
> > +	if (!xen_initial_domain() || HYPERVISOR_dom0_op(&op) < 0)
> > +		return NULL;
> > +
> > +	/* Here we know that Xen runs on EFI platform. */
> > +
> > +	efi = efi_xen;
> > +
> > +	op.cmd = XENPF_firmware_info;
> > +	op.u.firmware_info.type = XEN_FW_EFI_INFO;
> > +	op.u.firmware_info.index = XEN_FW_EFI_VENDOR;
> > +	info->vendor.bufsz = sizeof(vendor);
> > +	set_xen_guest_handle(info->vendor.name, vendor);
> > +
> > +	if (HYPERVISOR_dom0_op(&op) == 0) {
> > +		efi_systab_xen.fw_vendor = __pa_symbol(vendor);
> > +		efi_systab_xen.fw_revision = info->vendor.revision;
> > +	} else
> > +		efi_systab_xen.fw_vendor = __pa_symbol(L"UNKNOWN");
> > +
> > +	op.cmd = XENPF_firmware_info;
> > +	op.u.firmware_info.type = XEN_FW_EFI_INFO;
> > +	op.u.firmware_info.index = XEN_FW_EFI_VERSION;
> > +
> > +	if (HYPERVISOR_dom0_op(&op) == 0)
> > +		efi_systab_xen.hdr.revision = info->version;
> > +
> > +	op.cmd = XENPF_firmware_info;
> > +	op.u.firmware_info.type = XEN_FW_EFI_INFO;
> > +	op.u.firmware_info.index = XEN_FW_EFI_RT_VERSION;
> > +
> > +	if (HYPERVISOR_dom0_op(&op) == 0)
> > +		efi.runtime_version = info->version;
> > +
> > +	op.cmd = XENPF_firmware_info;
> > +	op.u.firmware_info.type = XEN_FW_EFI_INFO;
> > +	op.u.firmware_info.index = XEN_FW_EFI_CONFIG_TABLE;
> > +
> > +	if (HYPERVISOR_dom0_op(&op) < 0)
> > +		BUG();
>
> Is it really worth of a BUG()? Can't we just print a warning and return
> NULL? We could still boot without EFI support.

Earlier the same hypercall function succeeded so if here it failed
it means that something is really broken. However, I will try remove
this call and get all data from earlier one. This way we avoid this
BUG() call.

Daniel
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/