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
