Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

OC: Option for NVRAM persistence to enable boot-arg modification and SIP disable #575

Open
osy86 opened this issue Nov 15, 2019 · 41 comments
Open
Labels

Comments

@osy86
Copy link

@osy86 osy86 commented Nov 15, 2019

In the current implementation, we can add UEFI variables using the Add key under NVRAM. The way OcSetNvramVariable works is that it attempts to read the variable. If it's not found, then it will add it with the flags EFI_VARIABLE_BOOTSERVICE_ACCESS | EFI_VARIABLE_RUNTIME_ACCESS. Block can be used to delete the variable first in order to always force the Add value instead.

I propose a third option. Either a new key Persist that operates like Add but creates the variable with EFI_VARIABLE_NON_VOLATILE | EFI_VARIABLE_BOOTSERVICE_ACCESS | EFI_VARIABLE_RUNTIME_ACCESS. Or a new option PersistAdd with a boolean value that will mask EFI_VARIABLE_NON_VOLATILE for Add variables.

Why is this useful? Consider the following use case: We need some base set of boot-args to boot up OSX. So we add them with Add. However, in the process of development/testing unrelated stuff, we wish to modify boot-args by appending some new string. Using sudo nvram boot-args="new value" fails because OSX will try to use the attribute EFI_VARIABLE_NON_VOLATILE | EFI_VARIABLE_BOOTSERVICE_ACCESS | EFI_VARIABLE_RUNTIME_ACCESS and it fails because in the EFI specs:

If a preexisting variable is rewritten with different attributes, SetVariable() shall not modify the variable and shall return EFI_INVALID_PARAMETER.

Okay, so what if we just don't put boot-args into the config.plist. Instead we just set sudo nvram boot-args="some value" initially. That works, except if we require certain boot-args to boot, then forgetting to append them will make the system unbootable (even more painful if the vault is used). So ideally, we can use OC's "erase nvram" option to wipe all the variables and then reset them with a clean default.

A second use case: csr-active-config is used for SIP. OSX does the checks to make sure csr-active-config is only modified in recovery OS. However, setting it in recovery OS still doesn't work if there is already an Add entry. But if we do not include csr-active-config in both Add then using OC's "erase nvram" option will always enable SIP.

An option to allow the NVRAM options in config.plist to act as a signed/verified golden default configuration but still allow modification to NVRAM through normal OSX techniques would be useful.

@vit9696

This comment has been minimized.

Copy link
Contributor

@vit9696 vit9696 commented Nov 15, 2019

Hi,

Firstly, regarding the concept.

  • OpenCore NVRAM Add works mostly the way Mac firmwares work. It is meant to set the default variable values when they are missing due to previous NVRAM reset. This way you can get default user interface scaling (UIScale), enabled System Integrity Protection (csr-active-config), or many other variables like host name, backlight level, volume level, and so on.
  • OpenCore and kext projects in Acidanthera are built around the way that boot arguments are a temporary way of configuration, allowing you to quickly set a parameter through UEFI Shell. We do not believe any machine should need any boot arguments to work, if it does need our boot arguments, these should have DeviceProperties aliases.
  • OpenCore NVRAM Block concept is a hack or rather a security measure against the operating system. In certain cases we believe that the operating system cannot give enough protection for NVRAM variables. For example, Windows can modify csr-active-config at no issue in any mode. Currently FwRuntimeServices does not protect from that, and that is why we do provide an option to force some values.
  • OpenCore uses volatile variables to avoid accidentally triggering some driver bug. Many NVRAM drivers in laptops fail to reclaim space after resetting or deleting variables, so we have to workaround. Nothing changes if we use non-volatile variables, however, if there is any sensible reason to, we could consider.

Secondly, about the expected workflow.

  • The user has empty Block section (unless he wants that security I explained above part in), csr-active-config <00 00 00 00>, boot-args "-v".
  • Before using OpenCore the user resets NVRAM. When using a new computer this is not needed, but there also are no macOS variables, when migrating from e.g. Clover this is essential, as it leaves tons of garbage variables.
  • First start gives verbose mode and fully enabled SIP.
  • Then the user reboots to recovery and uses csrutil disable. This makes csr-active-config non-volatile, and sets it to non-zero value, effectively disabling SIP.
  • OpenCore no longer overrides csr-active-config at boot, but still provides -v for boot-args.
  • The user sets new boot-args, "-v batman=0xff". The same thing as with csrutil disable happens. The variable becomes non-volatile, OpenCore no longer sets it at next boot.
  • The user runs NVRAM cleaning with OpenCore. This removes all variables and effectively gives the user what he originally had. SIP is now fully on, and boot-args are back to -v. This is natural and matches Mac behaviour.

Given the above, I do not see an issue or the need to change anything. The logic looks perfectly natural to me.

@osy86

This comment has been minimized.

Copy link
Author

@osy86 osy86 commented Nov 15, 2019

Let me do some more tests and I will get back to you on the override behaviour I observed.

Regarding "We do not believe any machine should need any boot arguments to work", is there a device injection option for shikigva=32 shiki-id=Mac-BE088AF8C5EB4FA2?

@vit9696

This comment has been minimized.

Copy link
Contributor

@vit9696 vit9696 commented Nov 15, 2019

Regarding "We do not believe any machine should need any boot arguments to work", is there a device injection option for shikigva=32 shiki-id=Mac-BE088AF8C5EB4FA2?

That's a good point, should really implement them as IGPU properties. Could you submit a bugreport please?

If a preexisting variable is rewritten with different attributes, SetVariable() shall not modify the variable and shall return EFI_INVALID_PARAMETER.

I believe macOS firstly deletes variables. Or my firmware is non-conformant, as we tested the behaviour above and it worked. We could consider some routes to fix this in case it causes problems. One of them is to make FwRuntimeServices.efi delete volatile variables on set failure. @Download-Fritz, what do you think about it?

@Download-Fritz

This comment has been minimized.

Copy link

@Download-Fritz Download-Fritz commented Nov 16, 2019

@vit9696 This is a tough question because in the end it's a bug in Apple's nvram utility if it really does not delete the variables first. If you "fix" it in FwRuntimeServices, it will explicitly violate the spec. On the other hand, I don't see a good point for the specified behaviour either except as some sort of request sanitizing, imo wait for what @osy86 has to report later.

@osy86

This comment has been minimized.

Copy link
Author

@osy86 osy86 commented Nov 16, 2019

This is what I've tested

First boot:

$ sudo nvram boot-args
boot-args	alcid=11 shikigva=32 shiki-id=Mac-BE088AF8C5EB4FA2 -disablegfxfirmware
$ sudo nvram boot-args="alcid=11 shikigva=32 shiki-id=Mac-BE088AF8C5EB4FA2 -disablegfxfirmware test=test"
$ sudo nvram boot-args
boot-args	alcid=11 shikigva=32 shiki-id=Mac-BE088AF8C5EB4FA2 -disablegfxfirmware test=test

After rebooting:

$ sudo nvram boot-args
boot-args	alcid=11 shikigva=32 shiki-id=Mac-BE088AF8C5EB4FA2 -disablegfxfirmware

I've also reversed /System/Library/Extensions/AppleEFIRuntime.kext/Contents/PlugIns/AppleEFINVRAM.kext/Contents/MacOS/AppleEFINVRAM and looked into AppleEFINVRAM::setVariable (called by AppleEFINVRAM::setProperty) and could not find a delete before set. SetVariable is always called with EFI_VARIABLE_NON_VOLATILE | EFI_VARIABLE_BOOTSERVICE_ACCESS | EFI_VARIABLE_RUNTIME_ACCESS

@vit9696

This comment has been minimized.

Copy link
Contributor

@vit9696 vit9696 commented Nov 16, 2019

Thanks. In this case I do not mind ignoring the spec and incorporating the following change in SetVariable wrapper of FwRuntimeServices.efi.

  1. If SetVariable returned EFI_INVALID_PARAMETER and passed Attributes are EFI_VARIABLE_NON_VOLATILE | EFI_VARIABLE_BOOTSERVICE_ACCESS | EFI_VARIABLE_RUNTIME_ACCESS
  2. Call GetVariable with NULL buffer, zero size, and obtain current Attributes.
  3. If GetVariable returned EFI_BUFFER_TOO_SMALL and current Attributes are EFI_VARIABLE_BOOTSERVICE_ACCESS | EFI_VARIABLE_RUNTIME_ACCESS
  4. Call SetVariable with NULL buffer, zero size, and current Attributes.
  5. Call SetVariable with passed buffer, size, and Attributes (forward return).
@vit9696

This comment has been minimized.

Copy link
Contributor

@vit9696 vit9696 commented Nov 17, 2019

Implemented in

[master 269e18a] FwRuntimeServices: Added workaround for V to NV variable upgrade
 3 files changed, 43 insertions(+), 1 deletion(-)
@vit9696

This comment has been minimized.

Copy link
Contributor

@vit9696 vit9696 commented Dec 5, 2019

@osy86 can we close this now?

@osy86

This comment has been minimized.

Copy link
Author

@osy86 osy86 commented Dec 5, 2019

Sorry, will test tomorrow. Just got back tonight.

@osy86

This comment has been minimized.

Copy link
Author

@osy86 osy86 commented Dec 6, 2019

On latest release of OC and AppleSupport:

$ sudo nvram boot-args
boot-args	alcid=11 shikigva=32 shiki-id=Mac-BE088AF8C5EB4FA2 -disablegfxfirmware brcmfx-country=#a
$ sudo nvram boot-args="alcid=11 shikigva=32 shiki-id=Mac-BE088AF8C5EB4FA2 -disablegfxfirmware test=test"
nvram: Error setting variable - 'boot-args': (iokit/common) not permitted
@vit9696

This comment has been minimized.

Copy link
Contributor

@vit9696 vit9696 commented Dec 6, 2019

Are you positive this is not caused by System Integrity Protection or alike?

@osy86

This comment has been minimized.

Copy link
Author

@osy86 osy86 commented Dec 6, 2019

This is the same computer/configuration as #575 (comment) and also SIP shouldn't affect boot-args variable right

@vit9696

This comment has been minimized.

Copy link
Contributor

@vit9696 vit9696 commented Dec 6, 2019

From what I remember SIP does affect boot-args. You cannot e.g. add -s argument at the very least, and only a few boot arguments from a white list were allowed in older macOS versions. I do not know how it is now, but it should be like just -v or such. That is why NVIDIA was forced to stop using a boot argument for their web drivers for instance.

@osy86

This comment has been minimized.

Copy link
Author

@osy86 osy86 commented Dec 6, 2019

So creating a new variable still works. Setting a variable already marked NV (I used "prev-lang:kbd") still works. It seems only previously volatile variables now trigger that error instead of silently failing to change attribute.

That's strange to me. Looking at the edit, https://github.com/acidanthera/AppleSupportPkg/blob/master/Platform/FwRuntimeServices/UefiRuntimeServices.c#L981 this implies that previously Status == EFI_INVALID_PARAMETER otherwise this new code path wouldn't be taken. But in that case, I should have seen an error message before.

@osy86

This comment has been minimized.

@vit9696

This comment has been minimized.

Copy link
Contributor

@vit9696 vit9696 commented Dec 6, 2019

Variables in Apple Boot namespace, i.e. the ones accessed by nvram tool without the GUID prefix are not written immediately, but actually are cached and written at power off. Well, at least were :D. What you see here looks like a SIP check.

@vit9696

This comment has been minimized.

Copy link
Contributor

@vit9696 vit9696 commented Dec 6, 2019

As for the reasoning for the snippet, it removes the already present variable, you have to pass NULL value and size for that as per UEFI spec, and then sets a new one with different arguments. It is indeed the case that I did not check the error code for the first SetVariable, but most likely it is only good for debug reasons.

@osy86

This comment has been minimized.

Copy link
Author

@osy86 osy86 commented Dec 6, 2019

You're right, I must have tested with SIP disabled before. Now I get the same behaviour as before with SIP disabled (setting boot-args does not persist). Since the changes aren't committed until power off, perhaps the original issue isn't what I thought with the SetVariable but something is causing the change to not be written to NV storage.

@vit9696

This comment has been minimized.

Copy link
Contributor

@vit9696 vit9696 commented Dec 6, 2019

Perhaps your BIOS deviates from the specification and silently fails instead of returning invalid parameter? Could you try playing with the error condition maybe…

@osy86

This comment has been minimized.

Copy link
Author

@osy86 osy86 commented Dec 6, 2019

Yeah, I'll debug it more this weekend.

@osy86

This comment has been minimized.

Copy link
Author

@osy86 osy86 commented Dec 10, 2019

I'm testing with a new variable

		<key>Add</key>
		<dict>
			<key>7C436110-AB2A-4BBB-A880-FE41995C9F82</key>
			<dict>
				<key>xyz</key>
				<string>abc</string>
			</dict>
		</dict>

I replaced the code so it will always run:

    Status = mStoredGetVariable (
      VariableName,
      VendorGuid,
      &CurrAttributes,
      &CurrSize,
      NULL
      );
      mStoredSetVariable (
        VariableName,
        VendorGuid,
        CurrAttributes,
        0,
        NULL
        );
      Status = mStoredSetVariable (
        VariableName,
        VendorGuid,
        Attributes,
        DataSize,
        Data
        );

Boot into OSX with SIP disabled. sudo nvram xyz returns abc\0. Now I set sudo nvram xyz=123 and reboot. However, afterwards I see abc\0 again.

@Download-Fritz

This comment has been minimized.

Copy link

@Download-Fritz Download-Fritz commented Dec 10, 2019

Does the same happen during boot, i.e. if you call those functions with a hardcoded variable before OS start? What status do the first two calls return each (you could log to another var)?

@osy86

This comment has been minimized.

Copy link
Author

@osy86 osy86 commented Dec 10, 2019

  mStoredSetVariable (
    L"teststatus",
    VendorGuid,
    Attributes,
    sizeof(Status),
    &Status
    );

Should this create a variable named "teststatus"? If so, it's not working.

@Download-Fritz

This comment has been minimized.

Copy link

@Download-Fritz Download-Fritz commented Dec 10, 2019

Fun. I think it does not matter when calling the stored func, but is DisableVariableWrite on? Is it an ASUS board? If so, which model?

@osy86

This comment has been minimized.

Copy link
Author

@osy86 osy86 commented Dec 10, 2019

Okay this is bizarre. I went ahead and deleted the new code and changed the set to

  Status = mStoredSetVariable (
    VariableName,
    VendorGuid,
    Attributes | EFI_VARIABLE_NON_VOLATILE,
    DataSize,
    Data
    );

Explicating adding EFI_VARIABLE_NON_VOLATILE. Now it works as expected. This means that my original hypothesis (the EFI_VARIABLE_NON_VOLATILE getting ignored due to EFI specs) is wrong. Somehow, OSX isn't feeding EFI_VARIABLE_NON_VOLATILE into the attributes. Nobody else observe this behavior?

@vit9696

This comment has been minimized.

Copy link
Contributor

@vit9696 vit9696 commented Dec 10, 2019

@osy86 could you please elaborate the test you perform? It is a bit hard to judge from the contents.

@osy86

This comment has been minimized.

Copy link
Author

@osy86 osy86 commented Dec 10, 2019

So I use config.plist to set a variable named “xyz” and set it to “abc”. I don’t want to test with boot-args in case something else is affecting that variable. Then I use “sudo nvram xyz=123” to set that variable.

With latest OC and AppleSupportPkg, after rebooting, I see “xyz” set to “abc” again.

After modifying UefiRuntimeServices.c to force EFI_VARIABLE_NON_VOLATILE in the attribute param, I see “xyz” set to “123” as expected after reboot.

@vit9696

This comment has been minimized.

Copy link
Contributor

@vit9696 vit9696 commented Dec 10, 2019

Well, this test means that the variable was created as non volatile from the beginning, as FwRuntimeServices function in UEFI mode as well, so it does not change much.

I get a feeling it is as follows: volatile variables cannot be modified once added with your implementation once working from the OS. Modification includes not only updating with attribute change but also deleting and setting. Could you check this?

@osy86

This comment has been minimized.

Copy link
Author

@osy86 osy86 commented Dec 11, 2019

I removed the new code from UefiRuntimeServices and added the following to OcSetNvramVariable

  if (Status != EFI_BUFFER_TOO_SMALL) {
    Status = gRT->SetVariable (
      UnicodeVariableName,
      VariableGuid,
      OPEN_CORE_NVRAM_ATTR,
      VariableSize,
      VariableData
      );
    DEBUG ((
      EFI_ERROR (Status) ? DEBUG_WARN : DEBUG_INFO,
      "OC: Setting NVRAM %g:%a - %r\n",
      VariableGuid,
      AsciiVariableName,
      Status
      ));

    UINTN CurrSize;
    UINT32 CurrAttributes;
    char tmp[1024];
    CurrAttributes = 0;
    CurrSize = 0;
    Status = gRT->GetVariable (
      UnicodeVariableName,
      VariableGuid,
      NULL,
      &CurrSize,
      NULL
      );
    DEBUG ((
      EFI_ERROR (Status) ? DEBUG_WARN : DEBUG_INFO,
      "OC: Readback NVRAM %g:%a, size:%d - %r\n",
      VariableGuid,
      AsciiVariableName,
      CurrSize,
      Status
      ));
    Status = gRT->GetVariable (
      UnicodeVariableName,
      VariableGuid,
      &CurrAttributes,
      &CurrSize,
      tmp
      );
    DEBUG ((
      EFI_ERROR (Status) ? DEBUG_WARN : DEBUG_INFO,
      "OC: Readback NVRAM %g:%a, attributes:%d - %r\n",
      VariableGuid,
      AsciiVariableName,
      CurrAttributes,
      Status
      ));
    Status = gRT->SetVariable (
      UnicodeVariableName,
      VariableGuid,
      OPEN_CORE_NVRAM_ATTR | EFI_VARIABLE_NON_VOLATILE,
      VariableSize,
      VariableData
      );
    DEBUG ((
      EFI_ERROR (Status) ? DEBUG_WARN : DEBUG_INFO,
      "OC: Setting NVRAM (NV) %g:%a - %r\n",
      VariableGuid,
      AsciiVariableName,
      Status
      ));
    CurrAttributes = 0;
    CurrSize = 0;
    Status = gRT->GetVariable (
      UnicodeVariableName,
      VariableGuid,
      NULL,
      &CurrSize,
      NULL
      );
    DEBUG ((
      EFI_ERROR (Status) ? DEBUG_WARN : DEBUG_INFO,
      "OC: Readback NVRAM (NV) %g:%a, size:%d - %r\n",
      VariableGuid,
      AsciiVariableName,
      CurrSize,
      Status
      ));
    Status = gRT->GetVariable (
      UnicodeVariableName,
      VariableGuid,
      &CurrAttributes,
      &CurrSize,
      tmp
      );
    DEBUG ((
      EFI_ERROR (Status) ? DEBUG_WARN : DEBUG_INFO,
      "OC: Readback NVRAM (NV) %g:%a, attributes:%d - %r\n",
      VariableGuid,
      AsciiVariableName,
      CurrAttributes,
      Status
      ));
    Status = gRT->SetVariable (
      UnicodeVariableName,
      VariableGuid,
      OPEN_CORE_NVRAM_ATTR,
      0,
      NULL
      );
    DEBUG ((
      EFI_ERROR (Status) ? DEBUG_WARN : DEBUG_INFO,
      "OC: Setting NVRAM (Delete) %g:%a - %r\n",
      VariableGuid,
      AsciiVariableName,
      Status
      ));
    CurrAttributes = 0;
    CurrSize = 0;
    Status = gRT->GetVariable (
      UnicodeVariableName,
      VariableGuid,
      NULL,
      &CurrSize,
      NULL
      );
    DEBUG ((
      EFI_ERROR (Status) ? DEBUG_WARN : DEBUG_INFO,
      "OC: Readback NVRAM (Delete) %g:%a, size:%d - %r\n",
      VariableGuid,
      AsciiVariableName,
      CurrSize,
      Status
      ));
    Status = gRT->GetVariable (
      UnicodeVariableName,
      VariableGuid,
      &CurrAttributes,
      &CurrSize,
      tmp
      );
    DEBUG ((
      EFI_ERROR (Status) ? DEBUG_WARN : DEBUG_INFO,
      "OC: Readback NVRAM (Delete) %g:%a, attributes:%d - %r\n",
      VariableGuid,
      AsciiVariableName,
      CurrAttributes,
      Status
      ));
    Status = gRT->SetVariable (
      UnicodeVariableName,
      VariableGuid,
      OPEN_CORE_NVRAM_ATTR | EFI_VARIABLE_NON_VOLATILE,
      VariableSize,
      VariableData
      );
    DEBUG ((
      EFI_ERROR (Status) ? DEBUG_WARN : DEBUG_INFO,
      "OC: Setting NVRAM (NV) %g:%a - %r\n",
      VariableGuid,
      AsciiVariableName,
      Status
      ));
    CurrAttributes = 0;
    CurrSize = 0;
    Status = gRT->GetVariable (
      UnicodeVariableName,
      VariableGuid,
      NULL,
      &CurrSize,
      NULL
      );
    DEBUG ((
      EFI_ERROR (Status) ? DEBUG_WARN : DEBUG_INFO,
      "OC: Readback NVRAM (NV) %g:%a, size:%d - %r\n",
      VariableGuid,
      AsciiVariableName,
      CurrSize,
      Status
      ));
    Status = gRT->GetVariable (
      UnicodeVariableName,
      VariableGuid,
      &CurrAttributes,
      &CurrSize,
      tmp
      );
    DEBUG ((
      EFI_ERROR (Status) ? DEBUG_WARN : DEBUG_INFO,
      "OC: Readback NVRAM (NV) %g:%a, attributes:%d - %r\n",
      VariableGuid,
      AsciiVariableName,
      CurrAttributes,
      Status
      ));

    Status = gRT->SetVariable (
      UnicodeVariableName,
      VariableGuid,
      OPEN_CORE_NVRAM_ATTR,
      VariableSize,
      VariableData
      );
    DEBUG ((
      EFI_ERROR (Status) ? DEBUG_WARN : DEBUG_INFO,
      "OC: Setting NVRAM %g:%a - %r\n",
      VariableGuid,
      AsciiVariableName,
      Status
      ));

  }

This produced the following log

00:740 00:000 OC: Setting NVRAM (NV) 7C436110-AB2A-4BBB-A880-FE41995C9F82:xyz2 - Invalid Parameter
00:740 00:000 OC: Readback NVRAM (NV) 7C436110-AB2A-4BBB-A880-FE41995C9F82:xyz2, size:5 - Buffer Too Small
00:740 00:000 OC: Readback NVRAM (NV) 7C436110-AB2A-4BBB-A880-FE41995C9F82:xyz2, attributes:6 - Success
00:741 00:000 OC: Setting NVRAM (Delete) 7C436110-AB2A-4BBB-A880-FE41995C9F82:xyz2 - Success
00:741 00:000 OC: Readback NVRAM (Delete) 7C436110-AB2A-4BBB-A880-FE41995C9F82:xyz2, size:0 - Not Found
00:741 00:000 OC: Readback NVRAM (Delete) 7C436110-AB2A-4BBB-A880-FE41995C9F82:xyz2, attributes:0 - Not Found
00:743 00:001 OC: Setting NVRAM (NV) 7C436110-AB2A-4BBB-A880-FE41995C9F82:xyz2 - Success
00:743 00:000 OC: Readback NVRAM (NV) 7C436110-AB2A-4BBB-A880-FE41995C9F82:xyz2, size:5 - Buffer Too Small
00:744 00:000 OC: Readback NVRAM (NV) 7C436110-AB2A-4BBB-A880-FE41995C9F82:xyz2, attributes:7 - Success
00:744 00:000 OC: Setting NVRAM 7C436110-AB2A-4BBB-A880-FE41995C9F82:xyz2 - Invalid Parameter

So it is possible to delete and re-create as NVRAM. The issue is that the following doesn't work

    CurrSize = 0;
    Status = mStoredGetVariable (
      VariableName,
      VendorGuid,
      &CurrAttributes,
      &CurrSize,
      NULL
      );

When Status returns EFI_BUFFER_TOO_SMALL, CurrAttributes gets set to 0. This seems to be the spec behaviour BEFORE 2.8. Here is the entry from the revision table in 2.8 UEFI specs.

1954 set (*Attributes) when GetVariable() returns EFI_BUFFER_TOO_SMALL and Attributes is non-NULL

So back to WrapSetVariable this code doesn't work

    CurrSize = 0;
    Status = mStoredGetVariable (
      VariableName,
      VendorGuid,
      &CurrAttributes,
      &CurrSize,
      NULL
      );

    if (Status == EFI_BUFFER_TOO_SMALL
      && CurrAttributes == (EFI_VARIABLE_BOOTSERVICE_ACCESS | EFI_VARIABLE_RUNTIME_ACCESS)) {
      mStoredSetVariable (
        VariableName,
        VendorGuid,
        CurrAttributes,
        0,
        NULL
        );

CurrAttributes is not EFI_VARIABLE_BOOTSERVICE_ACCESS | EFI_VARIABLE_RUNTIME_ACCESS and even if we remove the if statement (from my first test), it still doesn't work because CurrAttributes is zero.

So to get the attributes of a variable complying to UEFI 2.7B, GetVariable must return EFI_SUCCESS. However, this means that it would have to read the actual variable which means there has to be a memory buffer. I'm not sure how you would like to do this efficiently.

EDIT: Should point out I did in fact test calling GetVariable with both valid Attribute pointer + non-zero size and Attribute pointer + zero size. My board does comply with 2.7B behaviour but NOT 2.8.

@vit9696

This comment has been minimized.

Copy link
Contributor

@vit9696 vit9696 commented Dec 11, 2019

Hrm, well, ok. I believe that querying UEFI runtime services with a 512 byte buffer is probably ok. I added a fallback in master. Does it work now?

@Download-Fritz

This comment has been minimized.

Copy link

@Download-Fritz Download-Fritz commented Dec 11, 2019

@osy86 Thank you for your detailed research!
@vit9696 I guess 512 bytes is plenty, but tbh we could also bruteforce the attributes to stay agnostic. There are not many options (tbh BS+RT(+NV) should suffice)

@vit9696

This comment has been minimized.

Copy link
Contributor

@vit9696 vit9696 commented Dec 11, 2019

@Download-Fritz how can you bruteforce without potentially exposing security vulnerabilities of the implementation? If it does not let to change a pair of attributes, that does not mean it will not prohibit changing all others.

@Download-Fritz

This comment has been minimized.

Copy link

@Download-Fritz Download-Fritz commented Dec 11, 2019

@vit9696 Well yes, I did not think it was FwRuntimeService's duty to make the implementation more secure. If you think of malicious code, well, the OS can do anything we can do too except for what we disallow by shimming, and I do not believe we have anything that'd prevent OS-level code to bruteforce like this itself. If you think of unintentional exploitation, that'd be interesting, but probably also pointless without more explicit counter measurements.

@vit9696

This comment has been minimized.

Copy link
Contributor

@vit9696 vit9696 commented Dec 11, 2019

Yes, talking about accidental exploitation, when the OS is not compromised and did valid stuff, but our code tried that bruteforce, and bypassed authenticated variables for instance.

@Download-Fritz

This comment has been minimized.

Copy link

@Download-Fritz Download-Fritz commented Dec 11, 2019

I don't think it provides significant protection, but I won't repel it till somebody runs into variable size issues.
@osy86 Can you please test and report?

@osy86

This comment has been minimized.

Copy link
Author

@osy86 osy86 commented Dec 11, 2019

Test code

  if (Status != EFI_BUFFER_TOO_SMALL) {
    Status = gRT->SetVariable (
      UnicodeVariableName,
      VariableGuid,
      OPEN_CORE_NVRAM_ATTR,
      VariableSize,
      VariableData
      );
    DEBUG ((
      EFI_ERROR (Status) ? DEBUG_WARN : DEBUG_INFO,
      "OC: Setting NVRAM %g:%a - %r\n",
      VariableGuid,
      AsciiVariableName,
      Status
      ));

    UINTN CurrSize;
    UINT32 CurrAttributes;
    char tmp[1024];
    CurrAttributes = 0;
    CurrSize = 0;
    Status = gRT->GetVariable (
      UnicodeVariableName,
      VariableGuid,
      &CurrAttributes,
      &CurrSize,
      NULL
      );
    DEBUG ((
      EFI_ERROR (Status) ? DEBUG_WARN : DEBUG_INFO,
      "OC: Readback NVRAM %g:%a, attributes:%d, size:%d - %r\n",
      VariableGuid,
      AsciiVariableName,
      CurrAttributes,
      CurrSize,
      Status
      ));
    CurrAttributes = 0;
    CurrSize = 512;
    Status = gRT->GetVariable (
      UnicodeVariableName,
      VariableGuid,
      &CurrAttributes,
      &CurrSize,
      tmp
      );
    DEBUG ((
      EFI_ERROR (Status) ? DEBUG_WARN : DEBUG_INFO,
      "OC: Readback NVRAM %g:%a, attributes:%d, size:%d - %r\n",
      VariableGuid,
      AsciiVariableName,
      CurrAttributes,
      CurrSize,
      Status
      ));

  }

Result

00:756 00:000 OC: Setting NVRAM 7C436110-AB2A-4BBB-A880-FE41995C9F82:xyz2 - Success
00:756 00:000 OC: Readback NVRAM 7C436110-AB2A-4BBB-A880-FE41995C9F82:xyz2, attributes:0, size:5 - Buffer Too Small
00:757 00:000 OC: Readback NVRAM 7C436110-AB2A-4BBB-A880-FE41995C9F82:xyz2, attributes:6, size:5 - Success
00:757 00:000 OC: Setting NVRAM 7C436110-AB2A-4BBB-A880-FE41995C9F82:xyz3 - Success
00:758 00:000 OC: Readback NVRAM 7C436110-AB2A-4BBB-A880-FE41995C9F82:xyz3, attributes:0, size:601 - Buffer Too Small
00:758 00:000 OC: Readback NVRAM 7C436110-AB2A-4BBB-A880-FE41995C9F82:xyz3, attributes:0, size:601 - Buffer Too Small

Now, I know it's unlikely for vars (such as boot-args) to be > 512 bytes. But that's not impossible, and in this case we regress to the old behaviour if the length is > an arbitrary number. If this is going to be the case, it would be good to have this behaviour documented somewhere just in case somebody runs into it.

@vit9696

This comment has been minimized.

Copy link
Contributor

@vit9696 vit9696 commented Dec 11, 2019

Yes, sure, I have just updated the docs to include this. This is fine to us, as basically this behaviour is rare, and most boards just allow writing non-volatile over volatile. Just a note, it sounds like a good idea to write to Intel and request them fix this.

@vit9696 vit9696 closed this Dec 11, 2019
@osy86

This comment has been minimized.

Copy link
Author

@osy86 osy86 commented Dec 11, 2019

Okay so this is weird. I tried the latest FwRuntimeServices and it still doesn't work. So I logged the status codes and https://github.com/acidanthera/AppleSupportPkg/blob/master/Platform/FwRuntimeServices/UefiRuntimeServices.c#L1018 still returns RETURN_INVALID_PARAMETER.

@vit9696

This comment has been minimized.

Copy link
Contributor

@vit9696 vit9696 commented Dec 11, 2019

Ergh, so you did not test it in runtime? Geh, now I do not like it. Could you please test further? In my opinion, the most likely case is that your firmware does not support creating non-volatile variables with the same name as volatile in runtime (only).

@vit9696 vit9696 reopened this Dec 11, 2019
@osy86

This comment has been minimized.

Copy link
Author

@osy86 osy86 commented Dec 11, 2019

Sorry, I was testing the 512 buffer first. I am going to add a Stall and try again.

the most likely case is that your firmware does not support creating non-volatile variables with the same name as volatile in runtime (only).

But my code above did work for creating such variable. The only difference is that I have a bunch of print statements in between.

EDIT: adding a delay didn't work. The delete (https://github.com/acidanthera/AppleSupportPkg/blob/master/Platform/FwRuntimeServices/UefiRuntimeServices.c#L1011) returns RETURN_INVALID_PARAMETER but for some reason, calling the same delete in OC's OcSetNvramVariable (right after the variable is created) does work.

@osy86

This comment has been minimized.

Copy link
Author

@osy86 osy86 commented Dec 11, 2019

Ugh, okay this is again documented in UEFI specs

Once ExitBootServices() is performed, only variables that have EFI_VARIABLE_RUNTIME_ACCESS and EFI_VARIABLE_NON_VOLATILE set can be set with SetVariable(). Variables that have runtime access but that are not nonvolatile are read- only data variables once ExitBootServices() is performed.

So if it wasn't set with EFI_VARIABLE_NON_VOLATILE before ExitBootServices, then it becomes RO and you cannot delete it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants
You can’t perform that action at this time.