From: James Bottomley <[email protected]>
The object here is to make the NV+BS variables accessible (at least read only)
at runtime so we can get a full picture of the state of the EFI variables for
debugging and secure boot purposes.
The way this is done is to get the efi stub to pull all the NV+BS
(i.e. variables without the RT flag) out into setup_data records which can
then be read back in at runtime. the EFI calls get_next_variable() and
get_variable() are modified to run through these setup records when the real
calls fail.
Signed-off-by: James Bottomley <[email protected]>
---
This same mechanism should work for all architectures, but the method of
transferring information from boot to runtime differs, so it can't be
done generically
James
diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
index c205035..6bbbb2e 100644
--- a/arch/x86/boot/compressed/eboot.c
+++ b/arch/x86/boot/compressed/eboot.c
@@ -251,6 +251,85 @@ static void find_bits(unsigned long mask, u8 *pos, u8 *size)
*size = len;
}
+static efi_status_t setup_efi_bootvars(struct boot_params *params)
+{
+ const int len = 1024;
+ efi_char16_t *str;
+ efi_status_t status;
+ struct setup_data **data = (struct setup_data **)¶ms->hdr.setup_data;
+
+ status = efi_call_phys3(sys_table->boottime->allocate_pool,
+ EFI_LOADER_DATA, len, &str);
+ if (status != EFI_SUCCESS)
+ return status;
+
+ /*
+ * Note: this trick only works on Little Endian if hdr.setup_data
+ * is u64 on both 64 and 32 bit
+ */
+ while (*data)
+ data = (struct setup_data **)&(*data)->next;
+
+ memset(str, 0, len);
+
+ for (;;) {
+ unsigned long size = len, str_len;
+ efi_guid_t guid;
+ struct efi_setup_bootvars *bvs;
+ unsigned int attributes;
+
+ status = efi_call_phys3(sys_table->runtime->get_next_variable,
+ &size, str, &guid);
+ if (status != EFI_SUCCESS)
+ break;
+
+ str_len = size;
+ size = 0;
+ status = efi_call_phys5(sys_table->runtime->get_variable,
+ str, &guid, &attributes, &size, NULL);
+ if (status != EFI_SUCCESS && status != EFI_BUFFER_TOO_SMALL)
+ break;
+
+
+ /*
+ * Please don't be tempted to optimise here: some
+ * UEFI implementations fail to set attributes if
+ * they return any error (including EFI_BUFFER_TOO_SMALL)
+ */
+ status = efi_call_phys3(sys_table->boottime->allocate_pool,
+ EFI_LOADER_DATA,
+ size + sizeof(*bvs) + str_len,
+ &bvs);
+ if (status != EFI_SUCCESS)
+ continue;
+ memset(bvs, 0 , sizeof(bvs));
+ status = efi_call_phys5(sys_table->runtime->get_variable,
+ str, &guid, &attributes,
+ &size, bvs->data);
+
+ if (status != EFI_SUCCESS
+ || (attributes & 0x07) != (EFI_VARIABLE_BOOTSERVICE_ACCESS |
+ EFI_VARIABLE_NON_VOLATILE)) {
+ efi_call_phys1(sys_table->boottime->free_pool, bvs);
+ continue;
+ }
+ memcpy(bvs->data + size, str, str_len);
+ bvs->name_size = str_len;
+ bvs->s_d.type = SETUP_EFIVAR;
+ bvs->s_d.len = size + sizeof(*bvs) + str_len;
+ bvs->guid = guid;
+ bvs->size = size;
+ bvs->attributes = attributes;
+ *data = (struct setup_data *)bvs;
+ data = (struct setup_data **)&(*data)->next;
+ }
+ *data = NULL;
+ status = EFI_SUCCESS;
+ out:
+ efi_call_phys1(sys_table->boottime->free_pool, str);
+ return status;
+}
+
static efi_status_t setup_efi_pci(struct boot_params *params)
{
efi_pci_io_protocol *pci;
@@ -1159,6 +1238,8 @@ struct boot_params *efi_main(void *handle, efi_system_table_t *_table,
setup_efi_pci(boot_params);
+ setup_efi_bootvars(boot_params);
+
status = efi_call_phys3(sys_table->boottime->allocate_pool,
EFI_LOADER_DATA, sizeof(*gdt),
(void **)&gdt);
diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h
index 60c89f3..2dadecd 100644
--- a/arch/x86/include/asm/efi.h
+++ b/arch/x86/include/asm/efi.h
@@ -122,4 +122,13 @@ static inline bool efi_is_native(void)
#define efi_call6(_f, _a1, _a2, _a3, _a4, _a5, _a6) (-ENOSYS)
#endif /* CONFIG_EFI */
+struct efi_setup_bootvars {
+ struct setup_data s_d;
+ unsigned int attributes;
+ efi_guid_t guid;
+ unsigned long name_size;
+ unsigned long size; /* size of the variable data (name follows) */
+ unsigned char data[0];
+};
+
#endif /* _ASM_X86_EFI_H */
diff --git a/arch/x86/include/uapi/asm/bootparam.h b/arch/x86/include/uapi/asm/bootparam.h
index c15ddaf..e864d23 100644
--- a/arch/x86/include/uapi/asm/bootparam.h
+++ b/arch/x86/include/uapi/asm/bootparam.h
@@ -6,6 +6,7 @@
#define SETUP_E820_EXT 1
#define SETUP_DTB 2
#define SETUP_PCI 3
+#define SETUP_EFIVAR 4
/* ram_size flags */
#define RAMDISK_IMAGE_START_MASK 0x07FF
diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
index fff986d..931060a 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -153,17 +153,103 @@ static efi_status_t virt_efi_get_variable(efi_char16_t *name,
unsigned long *data_size,
void *data)
{
- return efi_call_virt5(get_variable,
- name, vendor, attr,
- data_size, data);
+ efi_status_t status = efi_call_virt5(get_variable,
+ name, vendor, attr,
+ data_size, data);
+ u64 pa_data;
+ struct setup_data *s_d;
+
+ if (status != EFI_NOT_FOUND)
+ return status;
+
+
+ for (pa_data = boot_params.hdr.setup_data; pa_data;
+ pa_data = s_d->next) {
+ struct efi_setup_bootvars *bvs;
+ efi_char16_t *bvs_name;
+ int i = 0;
+
+ s_d = phys_to_virt(pa_data);
+
+ if (s_d->type != SETUP_EFIVAR)
+ continue;
+
+ bvs = (struct efi_setup_bootvars *)s_d;
+ bvs_name = (efi_char16_t *)(bvs->data + bvs->size);
+
+ for (i = 0; bvs_name[i] != 0 && name[i] != 0; i++)
+ if (bvs_name[i] != name[i])
+ break;
+
+ if (bvs_name[i] != 0 || name[i] != 0)
+ continue;
+
+ if (memcmp(&bvs->guid, vendor, sizeof(*vendor)) != 0)
+ continue;
+
+ if (attr)
+ *attr = bvs->attributes;
+
+ if (*data_size < bvs->size) {
+ *data_size = bvs->size;
+ return EFI_BUFFER_TOO_SMALL;
+ }
+ *data_size = bvs->size;
+ memcpy(data, bvs->data, bvs->size);
+
+ return EFI_SUCCESS;
+ }
+ return EFI_NOT_FOUND;
}
static efi_status_t virt_efi_get_next_variable(unsigned long *name_size,
efi_char16_t *name,
efi_guid_t *vendor)
{
- return efi_call_virt3(get_next_variable,
- name_size, name, vendor);
+ static int use_bootvars = 0;
+ efi_status_t status;
+ u64 pa_data = boot_params.hdr.setup_data;
+ int found = 0;
+ struct setup_data *data;
+
+ if (!use_bootvars) {
+ status = efi_call_virt3(get_next_variable,
+ name_size, name, vendor);
+ if (status == EFI_NOT_FOUND)
+ use_bootvars = 1;
+ else
+ return status;
+ }
+
+ for (pa_data = boot_params.hdr.setup_data; pa_data;
+ pa_data = data->next) {
+ struct efi_setup_bootvars *bvs;
+ efi_char16_t *bvs_name;
+
+ data = phys_to_virt(pa_data);
+
+ if (data->type != SETUP_EFIVAR)
+ continue;
+
+ bvs = (struct efi_setup_bootvars *)data;
+ bvs_name = (efi_char16_t *)(bvs->data + bvs->size);
+
+ if (use_bootvars != 1 && !found) {
+ unsigned long size = min(bvs->name_size, *name_size);
+
+ if (memcmp(bvs_name, name, size) == 0)
+ found = 1;
+ } else {
+ memcpy(name, bvs_name, bvs->name_size);
+ *name_size = bvs->name_size;
+ *vendor = bvs->guid;
+ use_bootvars = 2;
+ return EFI_SUCCESS;
+ }
+ }
+ use_bootvars = 0;
+
+ return EFI_NOT_FOUND;
}
static efi_status_t virt_efi_set_variable(efi_char16_t *name,
On Mon, Mar 18, 2013 at 08:40:14AM +0000, James Bottomley wrote:
> The object here is to make the NV+BS variables accessible (at least read only)
> at runtime so we can get a full picture of the state of the EFI variables for
> debugging and secure boot purposes.
I'd really prefer not to do this - the reason these aren't flagged as RT
is that they're not supposed to be visible at runtime and may break
certain security assumptions. If there's a real development purpose to
this then it ought to be guarded as a config option.
--
Matthew Garrett | [email protected]
On Tue, 2013-03-19 at 01:48 +0000, Matthew Garrett wrote:
> On Mon, Mar 18, 2013 at 08:40:14AM +0000, James Bottomley wrote:
>
> > The object here is to make the NV+BS variables accessible (at least read only)
> > at runtime so we can get a full picture of the state of the EFI variables for
> > debugging and secure boot purposes.
>
> I'd really prefer not to do this - the reason these aren't flagged as RT
> is that they're not supposed to be visible at runtime and may break
> certain security assumptions.
I suppose they could be flagged as read only by root, but I haven't seen
anything non-innocuous in them yet. It's mostly shell redirects and a
bit more useful information about the secure boot configuration.
Any security assumptions that rely on inability to read certain
information aren't really going to be that secure. Inability to modify,
sure, but inability to read, not really.
> If there's a real development purpose to
> this then it ought to be guarded as a config option.
I've no objection to them going under the secure boot lockdown config
option if that's what you're thinking.
James
On Tue, Mar 19, 2013 at 08:14:45AM +0000, James Bottomley wrote:
> Any security assumptions that rely on inability to read certain
> information aren't really going to be that secure. Inability to modify,
> sure, but inability to read, not really.
Well, I guess that's public/private key cryptography screwed.
--
Matthew Garrett | [email protected]
On Tue, 2013-03-19 at 16:35 +0000, Matthew Garrett wrote:
> On Tue, Mar 19, 2013 at 08:14:45AM +0000, James Bottomley wrote:
>
> > Any security assumptions that rely on inability to read certain
> > information aren't really going to be that secure. Inability to modify,
> > sure, but inability to read, not really.
>
> Well, I guess that's public/private key cryptography screwed.
Well, OK, it's ex-BIOS writers we're dealing with, so I won't say no-one
would be stupid enough to come up with a security scheme embedding
Private Keys in BS+NV variables, but I would have thought the fact that
Linux would blow the lid off it might be a good incentive not to do it
and thus a plus point for this patch.
James
On Tue, Mar 19, 2013 at 05:17:27PM +0000, James Bottomley wrote:
> On Tue, 2013-03-19 at 16:35 +0000, Matthew Garrett wrote:
> > On Tue, Mar 19, 2013 at 08:14:45AM +0000, James Bottomley wrote:
> >
> > > Any security assumptions that rely on inability to read certain
> > > information aren't really going to be that secure. Inability to modify,
> > > sure, but inability to read, not really.
> >
> > Well, I guess that's public/private key cryptography screwed.
>
> Well, OK, it's ex-BIOS writers we're dealing with, so I won't say no-one
> would be stupid enough to come up with a security scheme embedding
> Private Keys in BS+NV variables, but I would have thought the fact that
> Linux would blow the lid off it might be a good incentive not to do it
> and thus a plus point for this patch.
The hibernation scheme we'd discussed involved having the first stage
loader generating a keypair and handing half of it to the OS for
encryption of the hibernation partition, then handing the other half to
the OS on the next boot so it can decrypt it. That requires non-RT
variables to be restricted from OS visibility.
--
Matthew Garrett | [email protected]
On Tue, 2013-03-19 at 17:25 +0000, Matthew Garrett wrote:
> On Tue, Mar 19, 2013 at 05:17:27PM +0000, James Bottomley wrote:
> > On Tue, 2013-03-19 at 16:35 +0000, Matthew Garrett wrote:
> > > On Tue, Mar 19, 2013 at 08:14:45AM +0000, James Bottomley wrote:
> > >
> > > > Any security assumptions that rely on inability to read certain
> > > > information aren't really going to be that secure. Inability to modify,
> > > > sure, but inability to read, not really.
> > >
> > > Well, I guess that's public/private key cryptography screwed.
> >
> > Well, OK, it's ex-BIOS writers we're dealing with, so I won't say no-one
> > would be stupid enough to come up with a security scheme embedding
> > Private Keys in BS+NV variables, but I would have thought the fact that
> > Linux would blow the lid off it might be a good incentive not to do it
> > and thus a plus point for this patch.
>
> The hibernation scheme we'd discussed involved having the first stage
> loader generating a keypair and handing half of it to the OS for
> encryption of the hibernation partition, then handing the other half to
> the OS on the next boot so it can decrypt it. That requires non-RT
> variables to be restricted from OS visibility.
The scheme we discussed, unless something radically changed, was to
convey a temporary key pair via a mechanism to later verify the
hybernate kernel on a resume. That only requires reboot safe knowledge
of the public key. The private key can be conveyed in BS only (not NV),
and should be consumed (as in deleted) by the OS when it receives it, so
it wouldn't be exposed by this patch.
James
On Tue, Mar 19, 2013 at 06:23:31PM +0000, James Bottomley wrote:
> The scheme we discussed, unless something radically changed, was to
> convey a temporary key pair via a mechanism to later verify the
> hybernate kernel on a resume. That only requires reboot safe knowledge
> of the public key. The private key can be conveyed in BS only (not NV),
> and should be consumed (as in deleted) by the OS when it receives it, so
> it wouldn't be exposed by this patch.
It requires the key to survive the system being entirely powered down,
which means it needs to be BS+NV. It shouldn't be possible for userspace
to access this key.
--
Matthew Garrett | [email protected]
On Tue, 2013-03-19 at 18:28 +0000, Matthew Garrett wrote:
> On Tue, Mar 19, 2013 at 06:23:31PM +0000, James Bottomley wrote:
>
> > The scheme we discussed, unless something radically changed, was to
> > convey a temporary key pair via a mechanism to later verify the
> > hybernate kernel on a resume. That only requires reboot safe knowledge
> > of the public key. The private key can be conveyed in BS only (not NV),
> > and should be consumed (as in deleted) by the OS when it receives it, so
> > it wouldn't be exposed by this patch.
>
> It requires the key to survive the system being entirely powered down,
> which means it needs to be BS+NV. It shouldn't be possible for userspace
> to access this key.
It requires the *public* key to survive power down, certainly. The
private key can be thrown away once the hibernate image is signed. I
think the scheme can be constructed so the private key is never in NV
storage ... that also makes it more secure against tampering.
James
On Tue, Mar 19, 2013 at 06:40:56PM +0000, James Bottomley wrote:
> On Tue, 2013-03-19 at 18:28 +0000, Matthew Garrett wrote:
> > It requires the key to survive the system being entirely powered down,
> > which means it needs to be BS+NV. It shouldn't be possible for userspace
> > to access this key.
>
> It requires the *public* key to survive power down, certainly. The
> private key can be thrown away once the hibernate image is signed. I
> think the scheme can be constructed so the private key is never in NV
> storage ... that also makes it more secure against tampering.
Well, that somewhat complicates implementation - we'd be encrypting the
entire contents of memory except for the key that we're using to encrypt
memory. Keeping the public key away from userspace avoids having to care
about that.
--
Matthew Garrett | [email protected]
On Tue, 2013-03-19 at 18:50 +0000, Matthew Garrett wrote:
> On Tue, Mar 19, 2013 at 06:40:56PM +0000, James Bottomley wrote:
> > On Tue, 2013-03-19 at 18:28 +0000, Matthew Garrett wrote:
> > > It requires the key to survive the system being entirely powered down,
> > > which means it needs to be BS+NV. It shouldn't be possible for userspace
> > > to access this key.
> >
> > It requires the *public* key to survive power down, certainly. The
> > private key can be thrown away once the hibernate image is signed. I
> > think the scheme can be constructed so the private key is never in NV
> > storage ... that also makes it more secure against tampering.
>
> Well, that somewhat complicates implementation - we'd be encrypting the
> entire contents of memory except for the key that we're using to encrypt
> memory. Keeping the public key away from userspace avoids having to care
> about that.
I don't quite understand what you're getting at: the principle of public
key cryptography is that you can make the public key, well public. You
only need to guard the private key.
James
On Tue, Mar 19, 2013 at 11:00:31PM +0000, James Bottomley wrote:
> On Tue, 2013-03-19 at 18:50 +0000, Matthew Garrett wrote:
> > Well, that somewhat complicates implementation - we'd be encrypting the
> > entire contents of memory except for the key that we're using to encrypt
> > memory. Keeping the public key away from userspace avoids having to care
> > about that.
>
> I don't quite understand what you're getting at: the principle of public
> key cryptography is that you can make the public key, well public. You
> only need to guard the private key.
Ok, so let's just rephrase it as asymmetric cryptography. The aim is to
ensure that there's never a situation where userspace can decrypt a
hibernation file, modify it and reencrypt it. So, shim (or whatever)
generates a keypair. The encryption key is passed to the kernel being
booted. The decryption key is stashed in a variable in order to be
passed to the resume kernel.
If the decryption key is available to userspace then the kernel needs to
discard the encryption key during image write-out - otherwise the
encryption key will be in the encrypted image. If the decryption key
isn't available to userspace then this isn't a concern.
If the decryption key *is* available to userspace (as it would be in
your case), there's a requirement to discard the encryption key during
the hibernation process. This isn't impossible, but it does add a little
to the complexity.
--
Matthew Garrett | [email protected]
On Tue, 2013-03-19 at 23:17 +0000, Matthew Garrett wrote:
> On Tue, Mar 19, 2013 at 11:00:31PM +0000, James Bottomley wrote:
> > On Tue, 2013-03-19 at 18:50 +0000, Matthew Garrett wrote:
> > > Well, that somewhat complicates implementation - we'd be encrypting the
> > > entire contents of memory except for the key that we're using to encrypt
> > > memory. Keeping the public key away from userspace avoids having to care
> > > about that.
> >
> > I don't quite understand what you're getting at: the principle of public
> > key cryptography is that you can make the public key, well public. You
> > only need to guard the private key.
>
> Ok, so let's just rephrase it as asymmetric cryptography. The aim is to
> ensure that there's never a situation where userspace can decrypt a
> hibernation file, modify it and reencrypt it. So, shim (or whatever)
> generates a keypair. The encryption key is passed to the kernel being
> booted. The decryption key is stashed in a variable in order to be
> passed to the resume kernel.
>
> If the decryption key is available to userspace then the kernel needs to
> discard the encryption key during image write-out - otherwise the
> encryption key will be in the encrypted image. If the decryption key
> isn't available to userspace then this isn't a concern.
>
> If the decryption key *is* available to userspace (as it would be in
> your case), there's a requirement to discard the encryption key during
> the hibernation process. This isn't impossible, but it does add a little
> to the complexity.
I agree with this. But I do think the volatile secret key scheme, where
you discard the key immediately after use is the more secure one because
it relies on fewer secrets (and, indeed, no secrets at all after the
event). It's a case where transparency encourages better security
architecture.
James
On 03/18/2013 08:40 AM, James Bottomley wrote:
> From: James Bottomley <[email protected]>
>
> The object here is to make the NV+BS variables accessible (at least read only)
> at runtime so we can get a full picture of the state of the EFI variables for
> debugging and secure boot purposes.
This should definitely be guarded with a debug config option and off by
default. It needs to be made really clear that this code is for
debugging purposes.
[...]
> +static efi_status_t setup_efi_bootvars(struct boot_params *params)
> +{
> + const int len = 1024;
> + efi_char16_t *str;
> + efi_status_t status;
> + struct setup_data **data = (struct setup_data **)¶ms->hdr.setup_data;
> +
> + status = efi_call_phys3(sys_table->boottime->allocate_pool,
> + EFI_LOADER_DATA, len, &str);
> + if (status != EFI_SUCCESS)
> + return status;
> +
> + /*
> + * Note: this trick only works on Little Endian if hdr.setup_data
> + * is u64 on both 64 and 32 bit
> + */
> + while (*data)
> + data = (struct setup_data **)&(*data)->next;
> +
> + memset(str, 0, len);
> +
> + for (;;) {
> + unsigned long size = len, str_len;
> + efi_guid_t guid;
> + struct efi_setup_bootvars *bvs;
> + unsigned int attributes;
> +
> + status = efi_call_phys3(sys_table->runtime->get_next_variable,
> + &size, str, &guid);
> + if (status != EFI_SUCCESS)
> + break;
> +
> + str_len = size;
Beware of these recent debacles,
http://article.gmane.org/gmane.linux.kernel.efi/905
http://article.gmane.org/gmane.linux.kernel.efi/909
I'm not saying you need to handle them explicitly in this patch, at
least not if this is all under a config option with a big fat warning.
> + size = 0;
> + status = efi_call_phys5(sys_table->runtime->get_variable,
> + str, &guid, &attributes, &size, NULL);
> + if (status != EFI_SUCCESS && status != EFI_BUFFER_TOO_SMALL)
> + break;
> +
> +
> + /*
> + * Please don't be tempted to optimise here: some
> + * UEFI implementations fail to set attributes if
> + * they return any error (including EFI_BUFFER_TOO_SMALL)
> + */
> + status = efi_call_phys3(sys_table->boottime->allocate_pool,
> + EFI_LOADER_DATA,
> + size + sizeof(*bvs) + str_len,
> + &bvs);
> + if (status != EFI_SUCCESS)
> + continue;
Allocating things as EFI_LOADER_DATA is fine but you need to be sure to
reserve these memory regions so that they don't get used by any of the
memory allocators. See
http://article.gmane.org/gmane.linux.kernel.efi/580
(yes, setup_efi_pci() suffers from the same problem)
> + memset(bvs, 0 , sizeof(bvs));
> + status = efi_call_phys5(sys_table->runtime->get_variable,
> + str, &guid, &attributes,
> + &size, bvs->data);
> +
> + if (status != EFI_SUCCESS
> + || (attributes & 0x07) != (EFI_VARIABLE_BOOTSERVICE_ACCESS |
> + EFI_VARIABLE_NON_VOLATILE)) {
> + efi_call_phys1(sys_table->boottime->free_pool, bvs);
> + continue;
> + }
Please use the EFI_VARIABLE_* constants instead of the magic 0x07.
[...]
> diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
> index fff986d..931060a 100644
> --- a/arch/x86/platform/efi/efi.c
> +++ b/arch/x86/platform/efi/efi.c
> @@ -153,17 +153,103 @@ static efi_status_t virt_efi_get_variable(efi_char16_t *name,
> unsigned long *data_size,
> void *data)
> {
> - return efi_call_virt5(get_variable,
> - name, vendor, attr,
> - data_size, data);
> + efi_status_t status = efi_call_virt5(get_variable,
> + name, vendor, attr,
> + data_size, data);
> + u64 pa_data;
> + struct setup_data *s_d;
> +
> + if (status != EFI_NOT_FOUND)
> + return status;
> +
> +
> + for (pa_data = boot_params.hdr.setup_data; pa_data;
> + pa_data = s_d->next) {
> + struct efi_setup_bootvars *bvs;
> + efi_char16_t *bvs_name;
> + int i = 0;
> +
> + s_d = phys_to_virt(pa_data);
> +
> + if (s_d->type != SETUP_EFIVAR)
> + continue;
> +
> + bvs = (struct efi_setup_bootvars *)s_d;
> + bvs_name = (efi_char16_t *)(bvs->data + bvs->size);
> +
> + for (i = 0; bvs_name[i] != 0 && name[i] != 0; i++)
> + if (bvs_name[i] != name[i])
> + break;
> +
> + if (bvs_name[i] != 0 || name[i] != 0)
> + continue;
> +
This would be cleaner if you pulled some of the utf16_str* functions out
of drivers/firmware/efivars.c and stuck them in include/linux/efi.h.
Then you could do something like,
if (utf16_strncmp(bvs_name, name, utf16_strlen(name)) != 0)
continue;
> + if (memcmp(&bvs->guid, vendor, sizeof(*vendor)) != 0)
> + continue;
> +
> + if (attr)
> + *attr = bvs->attributes;
> +
> + if (*data_size < bvs->size) {
> + *data_size = bvs->size;
> + return EFI_BUFFER_TOO_SMALL;
> + }
> + *data_size = bvs->size;
> + memcpy(data, bvs->data, bvs->size);
> +
> + return EFI_SUCCESS;
> + }
> + return EFI_NOT_FOUND;
> }
>
> static efi_status_t virt_efi_get_next_variable(unsigned long *name_size,
> efi_char16_t *name,
> efi_guid_t *vendor)
> {
> - return efi_call_virt3(get_next_variable,
> - name_size, name, vendor);
> + static int use_bootvars = 0;
> + efi_status_t status;
> + u64 pa_data = boot_params.hdr.setup_data;
> + int found = 0;
Why not make found a boolean?
--
Matt Fleming, Intel Open Source Technology Center
On Wed, Mar 20, 2013 at 08:00:03AM +0000, James Bottomley wrote:
> I agree with this. But I do think the volatile secret key scheme, where
> you discard the key immediately after use is the more secure one because
> it relies on fewer secrets (and, indeed, no secrets at all after the
> event). It's a case where transparency encourages better security
> architecture.
That somewhat depends what you're securing against. There's an argument
that reusing this key is actually sufficiently secure so long as the
only way to obtain it is with physical access to the machine - that way
you don't have an nvram update cycle every boot. Exposing these
variables to userspace doesn't make it impossible to produce a secure
system, but it does remove some choices that were previously available.
--
Matthew Garrett | [email protected]