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

[Seiji Aguchi]

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