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

Update store APIs to also update HKLM #3660

Merged

Conversation

saxena-anurag
Copy link
Contributor

@saxena-anurag saxena-anurag commented Jun 21, 2024

Description

Issue:
When using libbpf APIs for attaching eBPF programs, the app / service needs to provide bpf_attach_type enum. ebpfapi.dll uses eBPF store to convert this enum to ebpf attach type guid. We recently took a change where we stopped populating eBPF store entries in HKLM, and they are now only updated in HKCU. That caused a regression and breaks any app / service that is running as LOCAL_SYSTEM.

Fix:
To mitigate the regressions, only in 0.17 release, update the store APIs to also attempt to update / delete / read from HKLM. If the update or delete operation fails due to ACCESS_DENIED, that error is absorbed, as the app may not be running as admin.

Testing

Existing tests.

Documentation

No.

Installation

Is there any installer impact for this change?

@saxena-anurag saxena-anurag changed the base branch from main to release/0.17 June 21, 2024 06:20
@saxena-anurag saxena-anurag reopened this Jun 21, 2024
@saxena-anurag saxena-anurag marked this pull request as ready for review June 21, 2024 14:28
@mtfriesen
Copy link
Collaborator

mtfriesen commented Jun 21, 2024

Documentation
No.

I realize this PR is only for release/0.17, but as an extension developer it is not clear which extension components are supposed to call the store APIs, when they are supposed to be called, and what the expected effects are. Please update the documentation to clearly define:

  1. When extension drivers should call the API, if at all, and what the expected effects are. Similarly, the effects of drivers registering their program and hook attributes through NMR, and which scenarios that unblocks.
  2. When extensions should register with the user mode store API, and what the effects are. For example, should extension installers typically register with the store API, or if the store affects only HKCU, who is responsible for registering and deregistering with the APIs?

See microsoft/xdp-for-windows#519, for example, where the expected behavior and effects are ambiguous.

@saxena-anurag
Copy link
Contributor Author

Documentation
No.

I realize this PR is only for release/0.17, but as an extension developer it is not clear which extension components are supposed to call the store APIs, when they are supposed to be called, and what the expected effects are. Please update the documentation to clearly define:

  1. When extension drivers should call the API, if at all, and what the expected effects are. Similarly, the effects of drivers registering their program and hook attributes through NMR, and which scenarios that unblocks.
  2. When extensions should register with the user mode store API, and what the effects are. For example, should extension installers typically register with the store API, or if the store affects only HKCU, who is responsible for registering and deregistering with the APIs?

See microsoft/xdp-for-windows#519, for example, where the expected behavior and effects are ambiguous.

There is no change in contract for the extension developers, only the internal implementation of the store APIs has been updated to also populate HKLM registry store.

  1. The extension drivers do not need to call the store APIs.
  2. Extension developers continue to publish a bpf_export EXE to call UM store APIs for updating and cleanup.

@saxena-anurag
Copy link
Contributor Author

Documentation
No.

I realize this PR is only for release/0.17, but as an extension developer it is not clear which extension components are supposed to call the store APIs, when they are supposed to be called, and what the expected effects are. Please update the documentation to clearly define:

  1. When extension drivers should call the API, if at all, and what the expected effects are. Similarly, the effects of drivers registering their program and hook attributes through NMR, and which scenarios that unblocks.
  2. When extensions should register with the user mode store API, and what the effects are. For example, should extension installers typically register with the store API, or if the store affects only HKCU, who is responsible for registering and deregistering with the APIs?

See microsoft/xdp-for-windows#519, for example, where the expected behavior and effects are ambiguous.

There is no change in contract for the extension developers, only the internal implementation of the store APIs has been updated to also populate HKLM registry store.

  1. The extension drivers do not need to call the store APIs.
  2. Extension developers continue to publish a bpf_export EXE to call UM store APIs for updating and cleanup.

Regarding updating documentation, agree that some parts need to be updated. That is currently tracked by #3021

@mtfriesen
Copy link
Collaborator

Extension developers continue to publish a bpf_export EXE to call UM store APIs for updating and cleanup.

When are extensions supposed to invoke their exe? Are they simply supposed to be provided, and any application or user that intends to use the extension directly invokes the extension exe?

@saxena-anurag
Copy link
Contributor Author

Extension developers continue to publish a bpf_export EXE to call UM store APIs for updating and cleanup.

When are extensions supposed to invoke their exe? Are they simply supposed to be provided, and any application or user that intends to use the extension directly invokes the extension exe?

I see what you mean. The "installation" step of the extension should invoke the exe to populate the store. Similarly, uninstallation should again invoke the exe to clear the store.

@mtfriesen
Copy link
Collaborator

mtfriesen commented Jun 21, 2024

The "installation" step of the extension should invoke the exe to populate the store. Similarly, uninstallation should again invoke the exe to clear the store.

Installation of an extension is a system-global operation: the driver can only load once, and even if it were started in parallel, only one instance of the driver can be chosen by eBPF through NMR.

The effect of registering with the ebpf store APIs on versions > 0.17, as I understand it, is limited to HKCU, i.e., the user who happens to install the extension.

Is this not a contradiction? An ideal software installer should have either global or user scope; having partially global and partially user scope will be confusing for users of the software.

@saxena-anurag
Copy link
Contributor Author

The "installation" step of the extension should invoke the exe to populate the store. Similarly, uninstallation should again invoke the exe to clear the store.

Installation of an extension is a system-global operation: the driver can only load once, and even if it were started in parallel, only one instance of the driver can be chosen by eBPF through NMR.

The effect of registering with the ebpf store APIs on versions > 0.17, as I understand it, is limited to HKCU, i.e., the user who happens to install the extension.

Is this not a contradiction? An ideal software installer should have either global or user scope; having partially global and partially user scope will be confusing for users of the software.

So this PR is patching 0.17 to also write to HKLM which should fix the issue for 0.17.x releases. For releases 0.18 onwards, we will need to either fix #3046 or port this PR to main.

mtfriesen
mtfriesen previously approved these changes Jun 21, 2024
@saxena-anurag saxena-anurag enabled auto-merge (squash) June 21, 2024 20:29
@saxena-anurag saxena-anurag changed the base branch from release/0.17 to main June 21, 2024 21:54
@saxena-anurag saxena-anurag dismissed mtfriesen’s stale review June 21, 2024 21:54

The base branch was changed.

@saxena-anurag saxena-anurag changed the base branch from main to release/0.17 June 21, 2024 21:54
@saxena-anurag
Copy link
Contributor Author

Moving to draft to investigate failures.

@saxena-anurag saxena-anurag marked this pull request as draft June 21, 2024 22:11
auto-merge was automatically disabled June 21, 2024 22:11

Pull request was converted to draft

@saxena-anurag saxena-anurag marked this pull request as ready for review June 22, 2024 02:00
@saxena-anurag saxena-anurag enabled auto-merge (squash) June 22, 2024 02:36
@saxena-anurag saxena-anurag merged commit 8682e77 into microsoft:release/0.17 Jun 22, 2024
83 of 85 checks passed
return result;
}

// Next delete from HKLM root key. It possible that the user does not have permission to delete from the HKLM root
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: 'It Is possible' (or it's possible)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants