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-efi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
