linux-kernel - RE: [RFC][PATCH v2] efivars,efi-pstore: Hold off deletion of sysfs entry until the scan is completed

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: <A5ED84D3BB3A384992CBB9C77DEDA4D443EF15D4@USINDEM103.corp.hds.com>
Date:	Fri, 11 Oct 2013 18:34:16 +0000
From:	Seiji Aguchi <seiji.aguchi@....com>
To:	Matt Fleming <matt@...sole-pimps.org>
CC:	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-efi@...r.kernel.org" <linux-efi@...r.kernel.org>,
	"tony.luck@...el.com" <tony.luck@...el.com>,
	"matt.fleming@...el.com" <matt.fleming@...el.com>,
	"dle-develop@...ts.sourceforge.net" 
	<dle-develop@...ts.sourceforge.net>,
	Tomoki Sekiyama <tomoki.sekiyama@....com>
Subject: RE: [RFC][PATCH v2] efivars,efi-pstore: Hold off deletion of sysfs
 entry until the scan is completed

Matt,

I submitted a v3 patch based on my comment below..

Seiji

> -----Original Message-----
> From: linux-efi-owner@...r.kernel.org [mailto:linux-efi-owner@...r.kernel.org] On Behalf Of Seiji Aguchi
> Sent: Wednesday, October 09, 2013 12:37 PM
> To: Matt Fleming
> Cc: linux-kernel@...r.kernel.org; linux-efi@...r.kernel.org; tony.luck@...el.com; matt.fleming@...el.com; dle-
> develop@...ts.sourceforge.net; Tomoki Sekiyama
> Subject: RE: [RFC][PATCH v2] efivars,efi-pstore: Hold off deletion of sysfs entry until the scan is completed
> 
> Thank you for reviewing.
> In my understanding, your point is that all accesses to efivar_entry should be done while holding __efivars->lock.
> 
> > > @@ -88,8 +103,9 @@ static int efi_pstore_read_func(struct efivar_entry *entry, void *data)
> > >  		return 0;
> > >
> > >  	entry->var.DataSize = 1024;
> > > -	__efivar_entry_get(entry, &entry->var.Attributes,
> > > -			   &entry->var.DataSize, entry->var.Data);
> > > +	efivar_entry_get(entry, &entry->var.Attributes,
> > > +			 &entry->var.DataSize, entry->var.Data);
> > > +
> > >  	size = entry->var.DataSize;
> > >
> > >  	*cb_data->buf = kmemdup(entry->var.Data, size, GFP_KERNEL);
> >
> > This isn't safe to do without holding the __efivars->lock, because
> > there's the potential for someone else to update entry->var.Data and
> > entry->var.DataSize while you're in the middle of copying the data in
> > kmemdup(). This could leak to an information leak, though I think you're
> > safe from an out-of-bounds access because DataSize is never > 1024.
> >
> 
> I see...
> Bu, kmemdup() cannot be called while holding the spinlock.
> 
> So, for protecting efivar_entry, I will call kzalloc() before holding the lock in efi_pstore_read().
> and use memcpy() in efi_pstore_read_func().
> 
> The pseudo code is as below.
> 
> 	static ssize_t efi_pstore_read(u64 *id, enum pstore_type_id *type,
>  				       struct pstore_info *psi)
>  	{
> 		*data.buf = kzalloc(1024, GFP_KERNEL);
> 		if (!*data.buf)
> 			return -ENOMEM;
> 
> 		efivar_entry_iter_begin();
> 		size = efi_pstore_sysfs_entry_iter(&data,
> 						   (struct efivar_entry **)&psi->data);
> 		efivar_entry_iter_end();
> 		if (size <= 0)
> 			kfree(*data.buf);
> 		return size;
>  	}
> 
> 	static int efi_pstore_read_func(struct efivar_entry *entry, void *data)
>  	{
> 	[...]
> 		entry->var.DataSize = 1024;
>  		__efivar_entry_get(entry, &entry->var.Attributes,
> 				 &entry->var.DataSize, entry->var.Data);
> 
>  		size = entry->var.DataSize;
> 		memcpy(*cb_data->buf, entry->var.Data, (size_t)min_t(unsigned long,
> 								     1024, size));
> 		return size;
> 	}
> 
> 
> > This doesn't look correct to me. You can't access 'entry' outside of the
> > *_iter_begin() and *_iter_end() blocks. You can't do,
> >
> > 	efivar_entry_iter_end():
> >
> > 	if (!entry->scanning)
> > 		efivar_unregister(entry);
> >
> > because 'entry' may have already been freed by another CPU.
> 
>  I will fix it as follows.
> 
> 	if (!entry->scanning) {
> 			efivar_entry_iter_end();
>  			efivar_unregister(entry);
> 	}  else
> 			efivar_entry_iter_end();
> 
> (efivar_unregister(entry) still runs concurrently.
> But, it cannot move inside spinlock because kzalloc() may run while freeing kobject.)
> 
> Is it your expectation?
> 
> Seiji
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-efi" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/