| 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
| ||
|
Date: Wed, 10 Apr 2013 15:25:54 +0000
From: Seiji Aguchi <seiji.aguchi@....com>
To: Matt Fleming <matt@...sole-pimps.org>,
"linux-efi@...r.kernel.org" <linux-efi@...r.kernel.org>
CC: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"Matt Fleming" <matt.fleming@...el.com>,
Tom Gundersen <teg@...m.no>,
"Matthew Garrett" <mjg59@...f.ucam.org>,
Jeremy Kerr <jeremy.kerr@...onical.com>,
"Tony Luck" <tony.luck@...el.com>,
Mike Waychison <mikew@...gle.com>
Subject: RE: [PATCH 3/6] efivars: efivar_entry API
> -static ssize_t efi_pstore_read(u64 *id, enum pstore_type_id *type,
> - int *count, struct timespec *timespec,
> - char **buf, struct pstore_info *psi)
> +struct pstore_read_data {
> + u64 *id;
> + enum pstore_type_id *type;
> + int *count;
> + struct timespec *timespec;
> + char **buf;
> +};
> +
> +static int efi_pstore_read_func(struct efivar_entry *entry, void *data)
> {
> efi_guid_t vendor = LINUX_EFI_CRASH_GUID;
> - struct efivars *efivars = __efivars;
> + struct pstore_read_data *cb_data = data;
> char name[DUMP_NAME_LEN];
> int i;
> int cnt;
> - unsigned int part, size;
> - unsigned long time;
> -
> - while (&efivars->walk_entry->list != &efivars->list) {
> - if (!efi_guidcmp(efivars->walk_entry->var.VendorGuid,
> - vendor)) {
> - for (i = 0; i < DUMP_NAME_LEN; i++) {
> - name[i] = efivars->walk_entry->var.VariableName[i];
> - }
> - if (sscanf(name, "dump-type%u-%u-%d-%lu",
> - type, &part, &cnt, &time) == 4) {
> - *id = part;
> - *count = cnt;
> - timespec->tv_sec = time;
> - timespec->tv_nsec = 0;
> - } else if (sscanf(name, "dump-type%u-%u-%lu",
> - type, &part, &time) == 3) {
> - /*
> - * Check if an old format,
> - * which doesn't support holding
> - * multiple logs, remains.
> - */
> - *id = part;
> - *count = 0;
> - timespec->tv_sec = time;
> - timespec->tv_nsec = 0;
> - } else {
> - efivars->walk_entry = list_entry(
> - efivars->walk_entry->list.next,
> - struct efivar_entry, list);
> - continue;
> - }
> + unsigned int part;
> + unsigned long time, size;
>
> - get_var_data_locked(efivars, &efivars->walk_entry->var);
> - size = efivars->walk_entry->var.DataSize;
> - *buf = kmalloc(size, GFP_KERNEL);
> - if (*buf == NULL)
> - return -ENOMEM;
> - memcpy(*buf, efivars->walk_entry->var.Data,
> - size);
> - efivars->walk_entry = list_entry(
> - efivars->walk_entry->list.next,
> - struct efivar_entry, list);
> - return size;
> - }
> - efivars->walk_entry = list_entry(efivars->walk_entry->list.next,
> - struct efivar_entry, list);
> - }
> - return 0;
> + if (efi_guidcmp(entry->var.VendorGuid, vendor))
> + return 0;
> +
> + for (i = 0; i < DUMP_NAME_LEN; i++)
> + name[i] = entry->var.VariableName[i];
> +
> + if (sscanf(name, "dump-type%u-%u-%d-%lu",
> + cb_data->type, &part, &cnt, &time) == 4) {
> + *cb_data->id = part;
> + *cb_data->count = cnt;
> + cb_data->timespec->tv_sec = time;
> + cb_data->timespec->tv_nsec = 0;
> + } else if (sscanf(name, "dump-type%u-%u-%lu",
> + cb_data->type, &part, &time) == 3) {
> + /*
> + * Check if an old format,
> + * which doesn't support holding
> + * multiple logs, remains.
> + */
> + *cb_data->id = part;
> + *cb_data->count = 0;
> + cb_data->timespec->tv_sec = time;
> + cb_data->timespec->tv_nsec = 0;
> + } else
> + return 0;
> +
> + efivar_entry_size(entry, &size);
Deadlocking will happen in this efivar_entry_size() because __efivars->lock is already hold
in efivar_entry_iter_begin().
> + *cb_data->buf = kmalloc(size, GFP_KERNEL);
> + if (*cb_data->buf == NULL)
> + return -ENOMEM;
> + memcpy(*cb_data->buf, entry->var.Data, size);
> + return size;
> +}
> +
> +static ssize_t efi_pstore_read(u64 *id, enum pstore_type_id *type,
> + int *count, struct timespec *timespec,
> + char **buf, struct pstore_info *psi)
> +{
> + struct pstore_read_data data;
> +
> + data.id = id;
> + data.type = type;
> + data.count = count;
> + data.timespec = timespec;
> + data.buf = buf;
> +
> + return __efivar_entry_iter(efi_pstore_read_func, &efivar_sysfs_list, &data,
> + (struct efivar_entry **)&psi->data);
> }
>
> static int efi_pstore_write(enum pstore_type_id type,
> @@ -1382,36 +1221,7 @@ static int efi_pstore_write(enum pstore_type_id type,
> char name[DUMP_NAME_LEN];
> efi_char16_t efi_name[DUMP_NAME_LEN];
> efi_guid_t vendor = LINUX_EFI_CRASH_GUID;
> - struct efivars *efivars = __efivars;
> int i, ret = 0;
> - efi_status_t status = EFI_NOT_FOUND;
> - unsigned long flags;
> -
> - if (pstore_cannot_block_path(reason)) {
> - /*
> - * If the lock is taken by another cpu in non-blocking path,
> - * this driver returns without entering firmware to avoid
> - * hanging up.
> - */
> - if (!spin_trylock_irqsave(&efivars->lock, flags))
> - return -EBUSY;
> - } else
> - spin_lock_irqsave(&efivars->lock, flags);
> -
> - /*
> - * Check if there is a space enough to log.
> - * size: a size of logging data
> - * DUMP_NAME_LEN * 2: a maximum size of variable name
> - */
> -
> - status = check_var_size_locked(efivars, PSTORE_EFI_ATTRIBUTES,
> - size + DUMP_NAME_LEN * 2);
> -
> - if (status) {
> - spin_unlock_irqrestore(&efivars->lock, flags);
> - *id = part;
> - return -ENOSPC;
> - }
>
> sprintf(name, "dump-type%u-%u-%d-%lu", type, part, count,
> get_seconds());
> @@ -1419,81 +1229,90 @@ static int efi_pstore_write(enum pstore_type_id type,
> for (i = 0; i < DUMP_NAME_LEN; i++)
> efi_name[i] = name[i];
>
> - efivars->ops->set_variable(efi_name, &vendor, PSTORE_EFI_ATTRIBUTES,
> - size, psi->buf);
> + ret = efivar_entry_set_safe(efi_name, vendor, PSTORE_EFI_ATTRIBUTES,
> + !pstore_cannot_block_path(reason),
> + size, psi->buf);
>
> - spin_unlock_irqrestore(&efivars->lock, flags);
> -
> - if (reason == KMSG_DUMP_OOPS && efivar_wq_enabled)
> + if (size && !ret && reason == KMSG_DUMP_OOPS && efivar_wq_enabled)
Why do you add (size && !ret) checking?
If the purpose of this patch is just adding new API, we don't need to modify the logic.
> schedule_work(&efivar_work);
>
> *id = part;
> return ret;
> };
--
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/
Powered by blists - more mailing lists