* [RFC] efivars write(2) races
@ 2013-01-25 0:25 Al Viro
2013-01-25 3:50 ` Lingzhu Xiang
2013-01-25 12:50 ` Matt Fleming
0 siblings, 2 replies; 7+ messages in thread
From: Al Viro @ 2013-01-25 0:25 UTC (permalink / raw)
To: Matt Fleming; +Cc: linux-efi, linux-kernel
1) process A does write() on efivars file, reaches ->get_variable(),
gets newdatasize set, drops efivars->lock and loses CPU before an attempt to
grab ->i_mutex. process B comes and does the same thing, replacing the
variable contents. Then it grabs ->i_mutex, updates size, drops ->i_mutex
and buggers off. At which point A gets CPU back and proceeds to set size
to whatever would be valid for its write. Only the value is bogus now...
2) what's to prevent EFI_NOT_FOUND being hit twice? Bad things
will obviously happen in that case...
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFC] efivars write(2) races
2013-01-25 0:25 [RFC] efivars write(2) races Al Viro
@ 2013-01-25 3:50 ` Lingzhu Xiang
2013-01-25 13:18 ` Matt Fleming
2013-01-25 12:50 ` Matt Fleming
1 sibling, 1 reply; 7+ messages in thread
From: Lingzhu Xiang @ 2013-01-25 3:50 UTC (permalink / raw)
To: Al Viro; +Cc: Matt Fleming, linux-efi, linux-kernel
On 01/25/2013 08:25 AM, Al Viro wrote:
> 1) process A does write() on efivars file, reaches ->get_variable(),
> gets newdatasize set, drops efivars->lock and loses CPU before an attempt to
> grab ->i_mutex. process B comes and does the same thing, replacing the
> variable contents. Then it grabs ->i_mutex, updates size, drops ->i_mutex
> and buggers off. At which point A gets CPU back and proceeds to set size
> to whatever would be valid for its write. Only the value is bogus now...
There are a few other things that makes size bogus now.
1. truncate() never touches nvram but pretends to be changing size.
2. Empty files come back with non-zero size after remount. They are imported
from sysfs when mounting.
3. Arguably reading empty files could just return empty instead of returning
EIO/EFI_NOT_FOUND from firmware.
4. EFI_VARIABLE_APPEND_WRITE with EFI_OUT_OF_RESOURCES truncates size but you
can still read its content.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFC] efivars write(2) races
2013-01-25 0:25 [RFC] efivars write(2) races Al Viro
2013-01-25 3:50 ` Lingzhu Xiang
@ 2013-01-25 12:50 ` Matt Fleming
1 sibling, 0 replies; 7+ messages in thread
From: Matt Fleming @ 2013-01-25 12:50 UTC (permalink / raw)
To: Al Viro; +Cc: linux-efi, linux-kernel, Jeremy Kerr, Lingzhu Xiang
On Fri, 2013-01-25 at 00:25 +0000, Al Viro wrote:
> 1) process A does write() on efivars file, reaches ->get_variable(),
> gets newdatasize set, drops efivars->lock and loses CPU before an attempt to
> grab ->i_mutex. process B comes and does the same thing, replacing the
> variable contents. Then it grabs ->i_mutex, updates size, drops ->i_mutex
> and buggers off. At which point A gets CPU back and proceeds to set size
> to whatever would be valid for its write. Only the value is bogus now...
Crap, yes. We'll have to jiggle the locking rules around to fix this
one.
> 2) what's to prevent EFI_NOT_FOUND being hit twice? Bad things
> will obviously happen in that case...
I'm not sure which execution path will trigger this? Oh, if we're
spinning for the spinlock in efivarfs_unlink() while another process
does a delete via efivarfs_file_write()? Yeah, that's gonna cause some
tears.
Thanks, Al.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFC] efivars write(2) races
2013-01-25 3:50 ` Lingzhu Xiang
@ 2013-01-25 13:18 ` Matt Fleming
2013-01-28 2:38 ` Lingzhu Xiang
0 siblings, 1 reply; 7+ messages in thread
From: Matt Fleming @ 2013-01-25 13:18 UTC (permalink / raw)
To: Lingzhu Xiang; +Cc: Al Viro, linux-efi, linux-kernel, Jeremy Kerr
On Fri, 2013-01-25 at 11:50 +0800, Lingzhu Xiang wrote:
> On 01/25/2013 08:25 AM, Al Viro wrote:
> > 1) process A does write() on efivars file, reaches ->get_variable(),
> > gets newdatasize set, drops efivars->lock and loses CPU before an attempt to
> > grab ->i_mutex. process B comes and does the same thing, replacing the
> > variable contents. Then it grabs ->i_mutex, updates size, drops ->i_mutex
> > and buggers off. At which point A gets CPU back and proceeds to set size
> > to whatever would be valid for its write. Only the value is bogus now...
>
> There are a few other things that makes size bogus now.
>
> 1. truncate() never touches nvram but pretends to be changing size.
Good catch. Looks like we need to provide an implementation for this.
> 2. Empty files come back with non-zero size after remount. They are imported
> from sysfs when mounting.
Eeek. The return value of get_variable() isn't checked in
efivarfs_fill_super().
> 3. Arguably reading empty files could just return empty instead of returning
> EIO/EFI_NOT_FOUND from firmware.
Yeah, I think Jeremy has a patch to fix this.
> 4. EFI_VARIABLE_APPEND_WRITE with EFI_OUT_OF_RESOURCES truncates size but you
> can still read its content.
I'm not sure what you mean by this. Could you please explain?
Thanks for the feedback.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFC] efivars write(2) races
2013-01-25 13:18 ` Matt Fleming
@ 2013-01-28 2:38 ` Lingzhu Xiang
2013-01-28 12:38 ` Matt Fleming
0 siblings, 1 reply; 7+ messages in thread
From: Lingzhu Xiang @ 2013-01-28 2:38 UTC (permalink / raw)
To: Matt Fleming; +Cc: Al Viro, linux-efi, linux-kernel, Jeremy Kerr
On 01/25/2013 09:18 PM, Matt Fleming wrote:
>> 4. EFI_VARIABLE_APPEND_WRITE with EFI_OUT_OF_RESOURCES truncates size but you
>> can still read its content.
>
> I'm not sure what you mean by this. Could you please explain?
Oops, this one is incorrect. I was testing EFI_VARIABLE_APPEND_WRITE
using this:
printf "\x47\x00\x00\x00\x00" >test-1-$guid
So it truncates the file before writing, but efivarfs figures out the right
size afterwards. The size remains zero if the write fails.
I should be using this for appending:
printf "\x47\x00\x00\x00\x00" >>test-1-$guid
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFC] efivars write(2) races
2013-01-28 2:38 ` Lingzhu Xiang
@ 2013-01-28 12:38 ` Matt Fleming
2013-01-29 2:30 ` Lingzhu Xiang
0 siblings, 1 reply; 7+ messages in thread
From: Matt Fleming @ 2013-01-28 12:38 UTC (permalink / raw)
To: Lingzhu Xiang; +Cc: Al Viro, linux-efi, linux-kernel, Jeremy Kerr
On Mon, 2013-01-28 at 10:38 +0800, Lingzhu Xiang wrote:
> On 01/25/2013 09:18 PM, Matt Fleming wrote:
> >> 4. EFI_VARIABLE_APPEND_WRITE with EFI_OUT_OF_RESOURCES truncates size but you
> >> can still read its content.
> >
> > I'm not sure what you mean by this. Could you please explain?
>
> Oops, this one is incorrect. I was testing EFI_VARIABLE_APPEND_WRITE
> using this:
>
> printf "\x47\x00\x00\x00\x00" >test-1-$guid
>
> So it truncates the file before writing, but efivarfs figures out the right
> size afterwards. The size remains zero if the write fails.
>
> I should be using this for appending:
>
> printf "\x47\x00\x00\x00\x00" >>test-1-$guid
Does this mean that these two test cases work as expected?
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFC] efivars write(2) races
2013-01-28 12:38 ` Matt Fleming
@ 2013-01-29 2:30 ` Lingzhu Xiang
0 siblings, 0 replies; 7+ messages in thread
From: Lingzhu Xiang @ 2013-01-29 2:30 UTC (permalink / raw)
To: Matt Fleming; +Cc: Al Viro, linux-efi, linux-kernel, Jeremy Kerr
On 01/28/2013 08:38 PM, Matt Fleming wrote:
> On Mon, 2013-01-28 at 10:38 +0800, Lingzhu Xiang wrote:
>> On 01/25/2013 09:18 PM, Matt Fleming wrote:
>>>> 4. EFI_VARIABLE_APPEND_WRITE with EFI_OUT_OF_RESOURCES truncates size but you
>>>> can still read its content.
>>>
>>> I'm not sure what you mean by this. Could you please explain?
>>
>> Oops, this one is incorrect. I was testing EFI_VARIABLE_APPEND_WRITE
>> using this:
>>
>> printf "\x47\x00\x00\x00\x00" >test-1-$guid
>>
>> So it truncates the file before writing, but efivarfs figures out the right
>> size afterwards. The size remains zero if the write fails.
>>
>> I should be using this for appending:
>>
>> printf "\x47\x00\x00\x00\x00" >>test-1-$guid
>
> Does this mean that these two test cases work as expected?
Mostly yes.
printf "\x47\x00\x00\x00\x00" >test-1-$guid
still shows unexpected truncate behavior. But this is the same one
already noted previously (truncate doesn't actually work).
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2013-01-29 2:43 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-01-25 0:25 [RFC] efivars write(2) races Al Viro
2013-01-25 3:50 ` Lingzhu Xiang
2013-01-25 13:18 ` Matt Fleming
2013-01-28 2:38 ` Lingzhu Xiang
2013-01-28 12:38 ` Matt Fleming
2013-01-29 2:30 ` Lingzhu Xiang
2013-01-25 12:50 ` Matt Fleming
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).