linux-kernel - Re: [PATCH v4 3/5] 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]

Message-Id: <537A26BB0200007800013AAE@mail.emea.novell.com>
Date:	Mon, 19 May 2014 14:43:55 +0100
From:	"Jan Beulich" <JBeulich@...e.com>
To:	"Daniel Kiper" <daniel.kiper@...cle.com>
Cc:	<david.vrabel@...rix.com>, <ian.campbell@...rix.com>,
	<stefano.stabellini@...citrix.com>, <jeremy@...p.org>,
	<matt.fleming@...el.com>, <x86@...nel.org>, <tglx@...utronix.de>,
	<xen-devel@...ts.xenproject.org>, <boris.ostrovsky@...cle.com>,
	<konrad.wilk@...cle.com>, <eshelton@...ox.com>, <mingo@...hat.com>,
	<mjg59@...f.ucam.org>, <linux-efi@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>, <hpa@...or.com>
Subject: Re: [PATCH v4 3/5] xen: Put EFI machinery in place

>>> On 16.05.14 at 22:41, <daniel.kiper@...cle.com> wrote:
> --- a/drivers/xen/Kconfig
> +++ b/drivers/xen/Kconfig
> @@ -240,4 +240,7 @@ config XEN_MCE_LOG
>  config XEN_HAVE_PVMMU
>         bool
>  
> +config XEN_EFI
> +	def_bool X86_64 && EFI

Constructs like this are bogus - they needlessly add a line to .config
when the expression evaluates to false.

config XEN_EFI
	def_bool y
	depends on X86_64 && EFI

is what avoids this.

> +static efi_status_t xen_efi_get_variable(efi_char16_t *name,
> +					 efi_guid_t *vendor,
> +					 u32 *attr,
> +					 unsigned long *data_size,
> +					 void *data)
> +{
> +	int err;
> +	DECLARE_CALL(get_variable);
> +
> +	set_xen_guest_handle(call.u.get_variable.name, name);
> +	BUILD_BUG_ON(sizeof(*vendor) !=
> +		     sizeof(call.u.get_variable.vendor_guid));
> +	memcpy(&call.u.get_variable.vendor_guid, vendor, sizeof(*vendor));
> +	call.u.get_variable.size = *data_size;
> +	set_xen_guest_handle(call.u.get_variable.data, data);
> +	err = HYPERVISOR_dom0_op(&op);
> +	if (err)
> +		return EFI_UNSUPPORTED;
> +
> +	*data_size = call.u.get_variable.size;
> +	*attr = call.misc; /* misc in struction is U32 variable*/

Iirc attr is an optional output, i.e. can be NULL, which hence needs
to be checked for here. I remember that this was broken in the
original EFI patch in our trees, so perhaps it would be good for you
to re-check against recent sources of ours for eventual other bug
fixes.

> +static efi_status_t xen_efi_query_variable_info(u32 attr,
> +						u64 *storage_space,
> +						u64 *remaining_space,
> +						u64 *max_variable_size)
> +{
> +	int err;
> +	DECLARE_CALL(query_variable_info);
> +
> +	if (efi.runtime_version < EFI_2_00_SYSTEM_TABLE_REVISION)
> +		return EFI_UNSUPPORTED;
> +

Here's a similar case - there is

	call.u.query_variable_info.attr = attr;

missing.

> +static efi_char16_t vendor[100] __initdata;
> +static const efi_char16_t unknown[] __initconst =
> +			{'U', 'N', 'K', 'N', 'O', 'W', 'N', '\0'};

If you enforced -fshort-wchar for the file's compilation, this could be
written in a more legible manner (and probably you wouldn't need a
named variable at all).

Jan

--
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/