Re: [PATCH v9 1/1] efi: a misc char interface for user to update efi firmware

[Borislav Petkov]

On Mon, Nov 02, 2015 at 06:47:29AM +, Kweh, Hock Leong wrote:
> By looking at your dmesg log, the above print out message seem that
> someone has called the flush() after the write(2). In my environment, flush()
> only being called in 2 places which are before write(2) and during close(2).
> The dmesg log seems that your environment is running write(2) and flush() in
> different threads and are parallel. Could you help me to double confirm this 
> and it 
> would be good if you could told me when the flush() is exactly being called in
> your environment. The info really help me on debugging.

I don't know what you mean: I simply do

cat /bin/ls > /dev/efi_capsule_loader

as root in an SMP kvm guest. And it explodes. Nothing special, just this
one command.

I guess you could try to reproduce it, here's how I start it:

qemu-system-x86_64
-enable-kvm
-gdb tcp::1234
-cpu Opteron_G5
-m 2048
-hda /home/boris/kvm/debian/sid-x86_64.img
-hdb /home/boris/kvm/swap.img
-boot menu=off,order=c
-localtime
-net nic,model=rtl8139
-net user,hostfwd=tcp::1235-:22
-usbdevice tablet
-kernel /home/boris/kernel/linux-2.6/arch/x86/boot/bzImage
-append "root=/dev/sda1 resume=/dev/sdb1 debug ignore_loglevel log_buf_len=16M 
earlyprintk=ttyS0,115200 console=ttyS0,115200 console=tty0"
-monitor pty
-virtfs local,path=/tmp,mount_tag=tmp,security_model=none
-serial file:/home/boris/kvm/test-x86_64-1235.log
-snapshot
-smp 8

HTH.

-- 
Regards/Gruss,
Boris.

ECO tip #101: Trim your mails when you reply.
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v9 1/1] efi: a misc char interface for user to update efi firmware

[Borislav Petkov]

On Mon, Nov 02, 2015 at 07:17:28AM +, Kweh, Hock Leong wrote:
> This is not a return value to indicate what is going now. It is a flag
> used in "cap_info->index" which positive value has a meaning of index
> number. I am using the negative value for the flag which similar to
> the implementation of pointer & error pointer (ERR_PTR).

Ok, but that doesn't make any sense: you're assigning UPLOAD_DONE to
cap_info->index only once in efi_capsule_submit_update() and you're not
testing it anywhere. Yeah, yeah, you're implicitly testing for it by
doing the "< 0" check.

So simply assign -1 to ->index to mean *any* type of error occurred,
remove the defines and you can always test for "< 0" to mean "did
something fail".

You simply don't need two error values...

-- 
Regards/Gruss,
Boris.

ECO tip #101: Trim your mails when you reply.
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] efi-pstore: fix kernel-doc argument name

[Kees Cook]

On Sat, Oct 31, 2015 at 8:23 AM, Geliang Tang  wrote:
> The first argument name in the kernel-doc argument list for
> efi_pstore_scan_sysfs_enter() was slightly off. Fix it for the
> kernel doc.
>
> Signed-off-by: Geliang Tang 

Acked-by: Kees Cook 

Thanks!

-Kees

> ---
>  drivers/firmware/efi/efi-pstore.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/firmware/efi/efi-pstore.c 
> b/drivers/firmware/efi/efi-pstore.c
> index c8d794c..eac76a7 100644
> --- a/drivers/firmware/efi/efi-pstore.c
> +++ b/drivers/firmware/efi/efi-pstore.c
> @@ -103,7 +103,7 @@ static int efi_pstore_read_func(struct efivar_entry 
> *entry, void *data)
>
>  /**
>   * efi_pstore_scan_sysfs_enter
> - * @entry: scanning entry
> + * @pos: scanning entry
>   * @next: next entry
>   * @head: list head
>   */
> --
> 2.4.3
>
>



-- 
Kees Cook
Chrome OS Security
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] efi: replace GFP_KERNEL with GFP_ATOMIC

[Matt Fleming]

On Wed, 28 Oct, at 09:12:27AM, Saurabh Sengar wrote:
> replace GFP_KERNEL with GFP_ATOMIC, as code while holding a spinlock
> should be atomic
> GFP_KERNEL may sleep and can cause deadlock, where as GFP_ATOMIC may
> fail but certainly avoids deadlock
> 
> Signed-off-by: Saurabh Sengar 
> ---
>  drivers/firmware/efi/vars.c | 12 +---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/firmware/efi/vars.c b/drivers/firmware/efi/vars.c
> index 70a0fb1..d4eeebf 100644
> --- a/drivers/firmware/efi/vars.c
> +++ b/drivers/firmware/efi/vars.c
> @@ -322,10 +322,11 @@ static unsigned long var_name_strnsize(efi_char16_t 
> *variable_name,
>   * disable the sysfs workqueue since the firmware is buggy.
>   */
>  static void dup_variable_bug(efi_char16_t *str16, efi_guid_t *vendor_guid,
> -  unsigned long len16)
> +  unsigned long len16, bool atomic)
>  {
>   size_t i, len8 = len16 / sizeof(efi_char16_t);
>   char *str8;
> + int gfp_mask;
>  
>   /*
>* Disable the workqueue since the algorithm it uses for
> @@ -334,7 +335,12 @@ static void dup_variable_bug(efi_char16_t *str16, 
> efi_guid_t *vendor_guid,
>*/
>   efivar_wq_enabled = false;
>  
> - str8 = kzalloc(len8, GFP_KERNEL);
> + if (atomic)
> + gfp_mask = GFP_ATOMIC;
> + else
> + gfp_mask = GFP_KERNEL;
> +
> + str8 = kzalloc(len8, gfp_mask);
>   if (!str8)
>   return;
>  
> @@ -408,7 +414,7 @@ int efivar_init(int (*func)(efi_char16_t *, efi_guid_t, 
> unsigned long, void *),
>   if (duplicates &&
>   variable_is_present(variable_name, _guid, 
> head)) {
>   dup_variable_bug(variable_name, _guid,
> -  variable_name_size);
> +  variable_name_size, atomic);
>   if (!atomic)
>   spin_lock_irq(&__efivars->lock);

It's slightly winding code, but if you look at the callers of
efivar_init() you'll see that none of them set both 'atomic' and
'duplicates', so dup_variable_bug() will never be called while holding
a spinlock.

Or am I missing something?
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html