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

Date:	Fri, 29 Jan 2016 01:22:30 +0000
From:	"Kweh, Hock Leong" <hock.leong.kweh@...el.com>
To:	Matt Fleming <matt@...eblueprint.co.uk>
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

> -----Original Message-----
> From: Matt Fleming [mailto:matt@...eblueprint.co.uk]
> Sent: Thursday, January 28, 2016 8:16 PM
> 
> 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.
> 

Ok. Noted. Will remove it and submit for v11.

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

Ok. Noted.

Thanks & Regards,
Wilson