linux-kernel - Re: [PATCH 3/6] efivars: efivar

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:	Thu, 11 Apr 2013 14:34:51 +0100
From:	Matt Fleming <matt@...sole-pimps.org>
To:	Seiji Aguchi <seiji.aguchi@....com>
CC:	"linux-efi@...r.kernel.org" <linux-efi@...r.kernel.org>,
	"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

On 10/04/13 16:25, Seiji Aguchi wrote:
>> +	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().

Good catch, thanks.

>> @@ -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.

That looks like a bug that slipped in. I'll fix it. Thanks!

-- 
Matt Fleming, Intel Open Source Technology Center
--
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/