[RFC] efivars write(2) races

[RFC] efivars write(2) races

[Al Viro]

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

Re: [RFC] efivars write(2) races

[Lingzhu Xiang]

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.

Re: [RFC] efivars write(2) races

[Matt Fleming]

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.

Re: [RFC] efivars write(2) races

[Lingzhu Xiang]

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

Re: [RFC] efivars write(2) races

[Matt Fleming]

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?

Re: [RFC] efivars write(2) races

[Lingzhu Xiang]

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

Re: [RFC] efivars write(2) races

[Matt Fleming]

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.