Jump to content

Clover Problems and Solutions

ErmaC

By ErmaC,
in Clover Development

 Share
3,206 posts in this topic

Recommended Posts

  • 2 weeks later...
vit9696
vit9696
Posted
Posted

May I request better support for bootercfg variable in Clover?

For some insane reason you write an integer to this variable, when it actually is a string similar to boot-args:

SetNvramVariable(L"bootercfg", &gAppleBootVariableGuid, Attributes, sizeof(gSettings.BooterConfig), &gSettings.BooterConfig);

 

The relevant details including the accepted values in bootercfg are described here:

http://www.insanelymac.com/forum/topic/331381-aptiomemoryfix/page-7?do=findComment&comment=2572819

It will be nice to have it configurable via the menu to get easier debug control.

Thanks :)

  • Like 6
Link to comment
Share on other sites
Philip Petev
Philip Petev
Posted
Posted

Guys, I'd like to raise this question once more: do we really need mtoc.NEW in /usr/local/bin? I remember the nasm path was changed in tools_def.txt to use the NASM_PREFIX variable. Is there any particular reason not to do the same with mtoc too? IMO, it would be better all those tools (nasm, mtoc, etc.) to be placed in one folder, avoiding the sudo usage.

I'm referring to the today's case of conflict between the mtoc binary, installed by the build script, bundled with AptioFixPkg and the symlinks, made by ebuild.sh/Build_Clover.command at the same place.

Link to comment
Share on other sites
nms
nms
Posted
Posted

Guys, I'd like to raise this question once more: do we really need mtoc.NEW in /usr/local/bin? I remember the nasm path was changed in tools_def.txt to use the NASM_PREFIX variable. Is there any particular reason not to do the same with mtoc too? IMO, it would be better all those tools (nasm, mtoc, etc.) to be placed in one folder, avoiding the sudo usage.

I'm referring to the today's case of conflict between the mtoc binary, installed by the build script, bundled with AptioFixPkg and the symlinks, made by ebuild.sh/Build_Clover.command at the same place.

Well,

all _PREFIX crotches are harmful. Looks like people did not know about PATH (and PACKAGES_PATH) environment variables.

 

Pity )-;

  • Like 1
Link to comment
Share on other sites
apianti
apianti
Posted
Posted

Guys, I'd like to raise this question once more: do we really need mtoc.NEW in /usr/local/bin? I remember the nasm path was changed in tools_def.txt to use the NASM_PREFIX variable. Is there any particular reason not to do the same with mtoc too? IMO, it would be better all those tools (nasm, mtoc, etc.) to be placed in one folder, avoiding the sudo usage.

I'm referring to the today's case of conflict between the mtoc binary, installed by the build script, bundled with AptioFixPkg and the symlinks, made by ebuild.sh/Build_Clover.command at the same place.

 

Make any relevant changes you want, especially if there is a conflict.

 

Well,

all _PREFIX crotches are harmful. Looks like people did not know about PATH (and PACKAGES_PATH) environment variables.

 

Pity )-;

 

Disagree. What if you have multiple versions of the same named binary installed? Like GCC for instance, you can't put all of them in PATH and expect that any but the first found would work. The prefix is there because you could have multiple versions installed. PACKAGES_PATH allows you to create multiple packages inside one EDK2 workspace under different file structures.

  • Like 1
Link to comment
Share on other sites
nms
nms
Posted
Posted

Make any relevant changes you want, especially if there is a conflict.

 

 

Disagree. What if you have multiple versions of the same named binary installed? Like GCC for instance, you can't put all of them in PATH and expect that any but the first found would work. The prefix is there because you could have multiple versions installed. PACKAGES_PATH allows you to create multiple packages inside one EDK2 workspace under different file structures.

The content of PATH is not carved in stone. One can easily use one day

 

PATH=/gcc1:$PATH sh

 

and another day

 

PATH=/gcc2:$PATH sh

 

You are the master of PATH.

 

As for PACKAGES_PATH You have an alternative view of it.

 

Documentation states that PACKAGES_PATH is ordered list of directories/folders to search for package(s). This dirs/folders can be anywhere. There is no reason to keep "foreign" packages in edk2 tree.

Link to comment
Share on other sites
apianti
apianti
Posted
Posted

The content of PATH is not carved in stone. One can easily use one day

 

PATH=/gcc1:$PATH sh

 

and another day

 

PATH=/gcc2:$PATH sh

 

You are the master of PATH.

 

This is the worst advice I've ever heard. You can entirely break your OS by modifying PATH incorrectly, even if by accident. It's just a super bad idea to change path to locate binaries that you never want in PATH because they are only used during building this source. Also there are paths to binaries in PATH that can't be overridden and therefore PATH won't help you anyway.

 

As for PACKAGES_PATH You have an alternative view of it.

 

Documentation states that PACKAGES_PATH is ordered list of directories/folders to search for package(s). This dirs/folders can be anywhere. There is no reason to keep "foreign" packages in edk2 tree.

 

That is exactly what I said. No idea where you got "foreign" from, I said that it allows any package paths to be included under one workspace.

 

Apple provides own nasm

NASM version 0.98.40 (Apple Computer, Inc. build 11) compiled on Nov  6 2017

We must not override it and not use it.

 

Yeah, I don't think he understands that PATH is actually for every executable on the system, not just the ones you currently want to use.

Link to comment
Share on other sites
nms
nms
Posted
Posted

This is the worst advice I've ever heard. You can entirely break your OS by modifying PATH incorrectly, even if by accident. It's just a super bad idea to change path to locate binaries that you never want in PATH because they are only used during building this source. Also there are paths to binaries in PATH that can't be overridden and therefore PATH won't help you anyway.

 

 

That is exactly what I said. No idea where you got "foreign" from, I said that it allows any package paths to be included under one workspace.

 

 

Yeah, I don't think he understands that PATH is actually for every executable on the system, not just the ones you currently want to use.

Looks like You missed the idea of subshell. )-;

Link to comment
Share on other sites
apianti
apianti
Posted
Posted

Looks like You missed the idea of subshell.

 

I didn't overlook anything, you did. If you change the PATH then all the other binaries that are relied on through PATH are no longer available because of the change you just made. You need the included GCC to build the cross compiler GCC. So you can't set the path because then you will only get the one that comes first but you need both. Plus there are others, as both slice and I have said now several times. There is a reason to use alternate variables for prefixes, they are used commonly.

 

EDIT: And without the prefix where does the cross compiler get built and installed?

Link to comment
Share on other sites
Philip Petev
Philip Petev
Posted
Posted

Looks like You missed the idea of subshell. )-;

We know the idea of subshell (at least I can speak for myself). I'm fully aware that changes, made to variables (by direct change of their values or exporting a new ones), remain valid only for the subshell, created by the currently running script. After the script has finished its job, it's back to normal as usual.

So my question is:

Does it really matter if we:

- append the PATH variable with the path to those tools

or

- use a separate "*_PREFIX"-type variables to set the path to them

or

- do anything else similar

when all these changes will remain until the script end? If it's all the same, then what kind of harm you're talking about?

Link to comment
Share on other sites
apianti
apianti
Posted
Posted

This is not the case anymore. cross-gcc is built with clang from xcode. xcrun is used to find SDK root, and then passed to configure param --with-sysroot

 

Well, clang is modified gcc and gcc is link to the same binary...... Also depends on how you build the cross compiler. But you are forgetting that there are also parts of the project built with the native compiler not the cross compiler. It's not a good idea to change PATH unless you expect that the binaries in that path should always be called, which is definitely not the case. You never know what another script/executable is going to try to find, it's best to point to what you need and let the rest be found like it was intended.

Link to comment
Share on other sites
Zenith432
Zenith432
Posted
Posted

Also depends on how you build the cross compiler.

For building the cross-compiler, only thing needed in the path is /usr/bin/xcrun, of which there is only one. It finds xcode SDK root based on xcode-select, which is stored somewhere in /private/var. Then GCC configure takes SDK root as parameter --with-sysroot and will create makefiles that use build tools only under SDK root, not anything found in path.
Link to comment
Share on other sites
apianti
apianti
Posted
Posted

For building the cross-compiler, only thing needed in the path is /usr/bin/xcrun, of which there is only one. It finds xcode SDK root based on xcode-select, which is stored somewhere in /private/var. Then GCC configure takes SDK root as parameter --with-sysroot and will create makefiles that use build tools only under SDK root, not anything found in path.

 

That's only one way to build the cross compiler. And you are incorrect xcrun modifies PATH for a subshell to point to the developer bin and SDK directories. Also it falls back to environmental variables to find that stuff....

Link to comment
Share on other sites
Zenith432
Zenith432
Posted
Posted

I'm talking about build_gcc7.sh.  It uses /usr/bin/xcrun only once to find the full pathname to xcode SDK root, which is stored in a file (or symlnk) somewhere under /private/var.  The full pathname to SDK root is then passed to GCC configure which generates makefiles with full pathnames for the build tools to use.

Link to comment
Share on other sites
apianti
apianti
Posted
Posted

I'm talking about build_gcc7.sh.  It uses /usr/bin/xcrun only once to find the full pathname to xcode SDK root, which is stored in a file (or symlnk) somewhere under /private/var.  The full pathname to SDK root is then passed to GCC configure which generates makefiles with full pathnames for the build tools to use.

 

As I said that is only one way to build the cross compiler. You can't make a decision because you think everyone is going to do it exactly how you want. What if I already have another GCC and want to use it to build the cross compiler?

 

EDIT: Here's a great example. NASM in macOS sucks, but it needs to be in PATH or fails to compile stuff. You need to compile another NASM and have both. You can't stick the one you compiled in PATH because the original is in there. You should be able to draw the parallels.

Link to comment
Share on other sites
apianti
apianti
Posted
Posted

Yes, I got that. I was trying to give you another example of why you need the prefixes. Because you need two different versions of some binaries. Yes, it's fine if you just use xcode to build the cross compiler, but if you use GCC (not from xcode) then you cannot add both to PATH. Therefore you cannot generate native and cross compiled without the prefix. Unless you are constantly going to switch back and forth between editing your PATH to change that one directory.

Link to comment
Share on other sites
  • 2 months later...
Slice
Slice
Posted
Posted

TODO.

1. We have a patch FixRegions but it works only in DSDT. In some cases Regions present in SSDTs and we should take them into account like 

GetBiosRegions(SSDT)

2. ArbitraryProperties should be rewritten to accept xml like gfxutil (DarwinDumper) 

	<key>PciRoot(0x0)/Pci(0x1b,0x0)</key>
	<dict>
		<key>PinConfigurations</key>
		<data>
		AA==
		</data>
		<key>layout-id</key>
		<data>
		DAAAAA==
		</data>
	</dict>
	<key>PciRoot(0x0)/Pci(0x1c,0x4)/Pci(0x0,0x0)</key>
	<dict>
		<key>built-in</key>
		<data>
		AQ==
		</data>
		<key>device_type</key>
		<string>ethernet</string>
	</dict>

3. FixAirport causes crash.

4. FixUSB is not good for PCIe XCHI controllers.

5. I propose to make special kind of DSDT binary patches like Rename. Such patch should search all occurrence of device _SB.PCI0.RP01.PXCS but not _SB.PCI0.RP02.PXCS assuming check for partial defines 

    Scope (_SB.PCI0)
    {
        Device (RP01)
        {
...

6. DropOEM_DSM can be excluded.

 

Any help from programmers will be appreciated.

  • Like 5
Link to comment
Share on other sites
  • 3 weeks later...
vit9696
vit9696
Posted
Posted (edited)

Slice, we have a serious problem in the hibernation code, which may lead to security risks.

 

We have been exploring hibernation specifics with lvs1974 and Download-Fritz, and realised that Apple makes a heavy use of RTC (CMOS) memory in both its kexts (AppleRTC.kext) and firmware (AppleBds.efi and probably many others through AppleRtcRam.efi). One of the use cases is passing the hibernation encryption key from XNU to the firmware.

 

AppleRTC.kext upon receiving the kPowerOff event via the exposed AppleRTC::setPowerState invokes AppleRTC::setHibernateState, which is responsible for saving IOHibernateRTCVariables IOReg variable for later use by the firmware.

 

Before writing it checks how much RAM does the current RTC controller have, and if there is less than 256 bytes of memory the whole write operation is aborted. That is why HibernationFixup is needed to manually save the IOHibernateRTCVariables to NVRAM when AppleRTC/FixRTC patches are used. These patches essentially reduce the reported memory to AppleRTC.kext (RTC: Only single RAM bank (128 bytes)) and thus effectively break hibernation. When AppleRTC.kext does not conflict with BIOS CMOS checksums these patches should obviously be avoided, but that is a side topic.

 

What is a real problem is that AppleRTC::setHibernateState effectively stores IOHibernateRTCVariables to 2 places:

— to NVRAM

— to higher 128 bytes of RTC memory

 

Apple writes to NVRAM flash due to some legacy reasons, and it is not our thing to deal with. However, the fact it writes to RTC is completely opaque to Clover, which does not know about a shadow copy of IOHibernateRTCVariables in the RTC memory. As a result this value, which is normally erased by AppleBds.efi, is never freed when booting with Clover, and exposes an encryption key to anybody able to read from RTC memory.

 

To properly fix things we should:

1. Switch to using RTC memory when reading the hibernation encryption key (Apple could ditch NVRAM saving soon).

2. Always erase hibernation encryption key memory region in RTC when booting. 

 

The following code could be used to access the RTC memory:

Spoiler 

#define R_PCH_RTC_INDEX           0x70
#define R_PCH_RTC_TARGET          0x71
#define R_PCH_RTC_EXT_INDEX       0x72
#define R_PCH_RTC_EXT_TARGET      0x73

#define R_PCH_RTC_INDEX_ALT       0x74
#define R_PCH_RTC_TARGET_ALT      0x75
#define R_PCH_RTC_EXT_INDEX_ALT   0x76
#define R_PCH_RTC_EXT_TARGET_ALT  0x77

UINT8
SimpleRtcRead (
  IN  UINT8  Offset
  )
{
  UINT8 RtcIndexPort;
  UINT8 RtcDataPort;

  // CMOS access registers (using alternative access not to handle NMI bit)
  if (Offset < 128) {
    RtcIndexPort  = R_PCH_RTC_INDEX_ALT;
    RtcDataPort   = R_PCH_RTC_TARGET_ALT;
  } else {
    RtcIndexPort  = R_PCH_RTC_EXT_INDEX_ALT;
    RtcDataPort   = R_PCH_RTC_EXT_TARGET_ALT;
  }

  IoWrite8 (RtcIndexPort, Offset & 0x7F);
  return IoRead8 (RtcDataPort);
}

VOID
SimpleRtcWrite (
  IN UINT8 Offset,
  IN UINT8 Value
  )
{
  UINT8  RtcIndexPort;
  UINT8  RtcDataPort;

  // CMOS access registers (using alternative access not to handle NMI bit)
  if (Offset < 128) {
    RtcIndexPort  = R_PCH_RTC_INDEX_ALT;
    RtcDataPort   = R_PCH_RTC_TARGET_ALT;
  } else {
    RtcIndexPort  = R_PCH_RTC_EXT_INDEX_ALT;
    RtcDataPort   = R_PCH_RTC_EXT_TARGET_ALT;
  }

  IoWrite8 (RtcIndexPort, Offset & 0x7F);
  IoWrite8 (RtcDataPort, Value);
}

 

 

And the following code could be used to access the variable itself:

  

#define APPLE_RTC_RAM_PROTOCOL_GUID \
  { 0xE121EC07, 0x9C42, 0x45EE, { 0xB0, 0xB6, 0xFF, 0xF8, 0xEF, 0x03, 0xC5, 0x21 } }

STATIC EFI_GUID mAppleRtcRamProtocolGuid = APPLE_RTC_RAM_PROTOCOL_GUID;

typedef struct _APPLE_RTC_RAM_PROTOCOL APPLE_RTC_RAM_PROTOCOL;

typedef UINT32 (EFIAPI *APPLE_RTC_RAM_GET_AVAILABLE_MEMORY) (
  IN APPLE_RTC_RAM_PROTOCOL    *This
  );

typedef EFI_STATUS (EFIAPI *APPLE_RTC_RAM_READ_MEMORY) (
  IN   APPLE_RTC_RAM_PROTOCOL  *This,
  OUT  UINT8                   *Data,
  IN   UINT32                  Size,
  IN   UINT32                  Offset
  );

typedef EFI_STATUS (EFIAPI *APPLE_RTC_RAM_WRITE_MEMORY) (
  IN   APPLE_RTC_RAM_PROTOCOL  *This,
  IN   UINT8                   *Data,
  IN   UINT32                  Size,
  IN   UINT32                  Offset
  );

typedef EFI_STATUS (EFIAPI *APPLE_RTC_RAM_ZERO_MEMORY) (
  IN   APPLE_RTC_RAM_PROTOCOL  *This
  );

typedef struct _APPLE_RTC_RAM_PROTOCOL {
  APPLE_RTC_RAM_GET_AVAILABLE_MEMORY   GetAvailableMemory;
  APPLE_RTC_RAM_READ_MEMORY            ReadMemory;
  APPLE_RTC_RAM_WRITE_MEMORY           WriteMemory;
  APPLE_RTC_RAM_ZERO_MEMORY            ZeroMemory;
} APPLE_RTC_RAM_PROTOCOL;

STATIC EFI_GUID mAppleBootVariableGuid = { 0x7C436110, 0xAB2A, 0x4BBB, { 0xA8, 0x80, 0xFE, 0x41, 0x99, 0x5C, 0x9F, 0x82 }};

#define EXPECTED_HIBERNATE_VAR_SIZE 44

STATIC
EFI_STATUS
ReadIORTCVariable (
  VOID
  )
{
  EFI_STATUS              Status;
  APPLE_RTC_RAM_PROTOCOL  *AppleRtc;
  UINT8                   *VariableData = NULL;
  UINTN                   VariableSize = 0;
  UINT32                  AvailableMemory;
  UINT8                   Data[EXPECTED_HIBERNATE_VAR_SIZE];
  UINT32                  Index;

  Print (L"Looking up APPLE_RTC_RAM_PROTOCOL...\n");

  Status = gBS->LocateProtocol (&mAppleRtcRamProtocolGuid, NULL, (VOID **)&AppleRtc);
  if (EFI_ERROR (Status)) {
    Print (L"Failed to find APPLE_RTC_RAM_PROTOCOL - %r\n", Status);

    Print (L"Falling back to SimpleRtcRead...\n");
    for (Index = 0; Index < EXPECTED_HIBERNATE_VAR_SIZE; Index++) {
      Data[Index] = SimpleRtcRead (Index + 128);
    }
  } else {
    AvailableMemory = AppleRtc->GetAvailableMemory (AppleRtc);

    Print (L"This RTC claims to have %d bytes of RAM!\n", AvailableMemory);

    if (AvailableMemory < 256) {
      Print (L"Upper 128 bytes of RTC RAM are not available!\n");
      return EFI_INVALID_PARAMETER;
    }

    Print (L"Loading IOHibernateRTCVariables from RTC...\n");

    Status = AppleRtc->ReadMemory (AppleRtc, Data, EXPECTED_HIBERNATE_VAR_SIZE, 128);
    if (EFI_ERROR (Status)) {
      Print (L"Failed to read IOHibernateRTCVariables from RTC - %r\n", Status);
      return Status;
    }
  }

  Print (L"Read IOHibernateRTCVariables from RTC has %08X magic\n", *(UINT32 *)Data);

  Print (L"Loading IOHibernateRTCVariables from NVRAM...\n");

  Status = GetVariable2 (L"IOHibernateRTCVariables", &mAppleBootVariableGuid, (VOID **)&VariableData, &VariableSize);
  if (EFI_ERROR (Status)) {
    Print (L"Failed to read IOHibernateRTCVariables from NVRAM - %r\n", Status);
    return Status;
  }

  if (VariableSize == EXPECTED_HIBERNATE_VAR_SIZE) {
    Print (L"Read IOHibernateRTCVariables from NVRAM has %08X magic\n", *(UINT32 *)VariableData);

    if (CompareMem (VariableData, Data, EXPECTED_HIBERNATE_VAR_SIZE) == 0) {
      Print (L"IOHibernateRTCVariables match in NVRAM and RTC\n");
    } else {
      Print (L"IOHibernateRTCVariables in NVRAM and RTC are different!!!\n");
    }

      Print (L"IOHibernateRTCVariables from RTC is as follows:\n");
      for (Index = 0; Index < EXPECTED_HIBERNATE_VAR_SIZE; Index++) {
        Print (L"%02X ", Data[Index]);
      }
      Print (L"\n");

      Print (L"IOHibernateRTCVariables from NVRAM is as follows:\n");
      for (Index = 0; Index < EXPECTED_HIBERNATE_VAR_SIZE; Index++) {
        Print (L"%02X ", VariableData[Index]);
      }
      Print (L"\n");

  } else {
    Status = EFI_INVALID_PARAMETER;
  }

  gBS->FreePool (VariableData);
  return Status;
}

 

 

Apple determines the validity of RTC hibernation encryption key area (44 bytes starting from the upper 128 bytes) by 41 41 50 4C hex sequence, and overwrites them with 44 45 41 44 hex sequence and all 40 zeroes.

Edited by vit9696
  • Like 8
Link to comment
Share on other sites
 Share
Go to topic listing
×
×
  • Create New...