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

[Seiji Aguchi]

Matt,

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

Seiji

> -----Original Message-----
> From: linux-efi-owner@xxxxxxxxxxxxxxx [mailto:linux-efi-owner@xxxxxxxxxxxxxxx] On Behalf Of Seiji Aguchi
> Sent: Wednesday, October 09, 2013 12:37 PM
> To: Matt Fleming
> Cc: linux-kernel@xxxxxxxxxxxxxxx; linux-efi@xxxxxxxxxxxxxxx; tony.luck@xxxxxxxxx; matt.fleming@xxxxxxxxx; dle-
> develop@xxxxxxxxxxxxxxxxxxxxx; 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@xxxxxxxxxxxxxxx
> 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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/