linux-kernel - Re: [PATCH v10 1/1] efi: a misc char interface for user to update efi firmware

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] [day] [month] [year] [list]

Message-ID: <20160128121602.GD2571@codeblueprint.co.uk>
Date:	Thu, 28 Jan 2016 12:16:02 +0000
From:	Matt Fleming <matt@...eblueprint.co.uk>
To:	"Kweh, Hock Leong" <hock.leong.kweh@...el.com>
Cc:	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	"Ong, Boon Leong" <boon.leong.ong@...el.com>,
	LKML <linux-kernel@...r.kernel.org>,
	"linux-efi@...r.kernel.org" <linux-efi@...r.kernel.org>,
	Sam Protsenko <semen.protsenko@...aro.org>,
	Peter Jones <pjones@...hat.com>,
	Andy Lutomirski <luto@...capital.net>,
	Roy Franz <roy.franz@...aro.org>,
	Borislav Petkov <bp@...en8.de>,
	James Bottomley <James.Bottomley@...senpartnership.com>,
	Linux FS Devel <linux-fsdevel@...r.kernel.org>,
	"Anvin, H Peter" <h.peter.anvin@...el.com>
Subject: Re: [PATCH v10 1/1] efi: a misc char interface for user to update
 efi firmware

On Tue, 26 Jan, at 03:10:03AM, Kweh Hock Leong wrote:
> > 
> > This mutex is not needed. It doesn't protect anything in your code.
> 
> This is to synchronize/serializes one of the instant is calling efi_capsule_supported()
> and the other one is calling efi_capsule_update() which they are exported symbol
> from capsule.c.
 
You don't need to synchronise them.

Looking at my original capsule patches I can see why you might be
doing this locking, but it's unnecessary. You don't need to serialise
access to efi_capsule_supported() and efi_capsule_update().

Internally to the core EFI capsule code we need to ensure that we
don't allow capsules with conflicting reset types to be sent to the
firmware (how would we know the type of reset to perform?), but that
should be entirely transparent to your driver.  

I'm planning on re-sending my capsule patches, so all this should
become clearer.

> > This function would be much more simple if you handled the above
> > condition differently.
> > 
> > Instead of having code in efi_capsule_setup_info() to allocate the
> > initial ->pages memory, why not just allocate it in efi_capsule_open()
> > at the same time as you allocate the private_data? You can then
> > free it in efi_capsule_release() (you're leaking it at the moment).
> > 
> > You are always going to need at least one element in ->pages for
> > successful operation, so you might as well allocate it up front.
> 
> Just want to double check I understand you correctly. Do you mean
> I should free ->pages during close(2) but not free the ->pages[X] if
> there is any successfully submitted?

Yes, that's correct.