linux-kernel - Re: [PATCH] efi/efivars: Create efivars mount point in the registration of efivars abstraction

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: <20200925004315.GC31226@linux-l9pv.suse>
Date:   Fri, 25 Sep 2020 08:43:15 +0800
From:   joeyli <jlee@...e.com>
To:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc:     "Lee, Chun-Yi" <joeyli.kernel@...il.com>,
        Ard Biesheuvel <ard.biesheuvel@...aro.org>,
        linux-efi@...r.kernel.org, linux-kernel@...r.kernel.org,
        Ard Biesheuvel <ardb@...nel.org>,
        Matthias Brugger <mbrugger@...e.com>,
        Fabian Vogt <fvogt@...e.com>,
        Ilias Apalodimas <ilias.apalodimas@...aro.org>,
        Arthur Heymans <arthur@...ymans.xyz>,
        Patrick Rudolph <patrick.rudolph@...ements.com>
Subject: Re: [PATCH] efi/efivars: Create efivars mount point in the
 registration of efivars abstraction

Hi Greg,

On Thu, Sep 24, 2020 at 11:51:57AM +0200, Greg Kroah-Hartman wrote:
> On Thu, Sep 24, 2020 at 04:28:33PM +0800, Lee, Chun-Yi wrote:
> > This patch moved the logic of creating efivars mount point to the
> > registration of efivars abstraction. It's useful for userland to
> > determine the availability of efivars filesystem by checking the
> > existence of mount point.
> 
> Why not do what all other tools do, and look in /proc/filesystems?
> 
> Why is efivars "special" in this way?  What tool isn't properly looking
> for the filesystem in that way today?
> 

Thanks for your idea. I think that this is good enough for userland
tool to check the availability of efivarfs. Ignore my patch please.

Regards
Joey Lee

> > The 'efivars' platform device be created on generic EFI runtime services
> > platform, so it can be used to determine the availability of efivarfs.
> > But this approach is not available for google gsmi efivars abstraction.
> 
> I do not understand this last sentence, can you try to explain it
> better?
> 
> > This patch be tested on Here on qemu-OVMF and qemu-uboot.
> 
> How about real hardware?  :)
> 
> > 
> > Cc: Ard Biesheuvel <ardb@...nel.org>
> > Cc: Matthias Brugger <mbrugger@...e.com>
> > Cc: Fabian Vogt <fvogt@...e.com>
> > Cc: Ilias Apalodimas <ilias.apalodimas@...aro.org>
> > Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
> > Cc: Arthur Heymans <arthur@...ymans.xyz>
> > Cc: Patrick Rudolph <patrick.rudolph@...ements.com>
> > Signed-off-by: "Lee, Chun-Yi" <jlee@...e.com>
> > ---
> >  drivers/firmware/efi/efi.c  |  7 -------
> >  drivers/firmware/efi/vars.c | 17 +++++++++++++++++
> >  2 files changed, 17 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> > index 3aa07c3b5136..23c11a2a3f4d 100644
> > --- a/drivers/firmware/efi/efi.c
> > +++ b/drivers/firmware/efi/efi.c
> > @@ -405,13 +405,6 @@ static int __init efisubsys_init(void)
> >  	if (error)
> >  		goto err_remove_group;
> >  
> > -	/* and the standard mountpoint for efivarfs */
> > -	error = sysfs_create_mount_point(efi_kobj, "efivars");
> > -	if (error) {
> > -		pr_err("efivars: Subsystem registration failed.\n");
> > -		goto err_remove_group;
> > -	}
> > -
> >  	if (efi_enabled(EFI_DBG) && efi_enabled(EFI_PRESERVE_BS_REGIONS))
> >  		efi_debugfs_init();
> >  
> > diff --git a/drivers/firmware/efi/vars.c b/drivers/firmware/efi/vars.c
> > index 973eef234b36..6fa7f288d635 100644
> > --- a/drivers/firmware/efi/vars.c
> > +++ b/drivers/firmware/efi/vars.c
> > @@ -1179,6 +1179,8 @@ int efivars_register(struct efivars *efivars,
> >  		     const struct efivar_operations *ops,
> >  		     struct kobject *kobject)
> >  {
> > +	int error;
> > +
> >  	if (down_interruptible(&efivars_lock))
> >  		return -EINTR;
> >  
> > @@ -1191,6 +1193,19 @@ int efivars_register(struct efivars *efivars,
> >  
> >  	up(&efivars_lock);
> >  
> > +	/* and the standard mountpoint for efivarfs */
> > +	if (efi_kobj) {
> 
> Why test for this?  Can it race?
> 
> thanks,
> 
> greg k-h