Skip to content

Add features for the Host Security ID program#660

Merged
vathpela merged 6 commits intorhboot:mainfrom
vathpela:hsi
Feb 24, 2025
Merged

Add features for the Host Security ID program#660
vathpela merged 6 commits intorhboot:mainfrom
vathpela:hsi

Conversation

@vathpela
Copy link
Member

No description provided.

@vathpela vathpela force-pushed the hsi branch 2 times, most recently from f75a896 to b446211 Compare May 15, 2024 20:39
@kukrimate
Copy link
Collaborator

Shouldn't this also explicitly expose if data sections of the image are executable or is that implied by heap is executable flag?

@dennis-tseng99
Copy link
Contributor

May I suggest you could put error codes on the bottom of get_hsi_mem_info() ?

void
get_hsi_mem_info(void)
{
	EFI_STATUS efi_status;
	uintptr_t addr;
	uint64_t attrs = 0;
	uint32_t *tmp_alloc;

	addr = ((uintptr_t)&get_hsi_mem_info) & ~EFI_PAGE_MASK;
	efi_status = get_mem_attrs(addr, EFI_PAGE_SIZE, &attrs);
	if (EFI_ERROR(efi_status)) {
                goto error;
        }

	hsi_status = SHIM_HSI_STATUS_HASMAP;
	if (attrs & MEM_ATTR_W) {
		hsi_status |= SHIM_HSI_STATUS_ROW;
	}

	addr = ((uintptr_t)&addr) & ~EFI_PAGE_MASK;
	efi_status = get_mem_attrs(addr, EFI_PAGE_SIZE, &attrs);
	if (EFI_ERROR(efi_status)) {
		goto error;
	}

	if (attrs & MEM_ATTR_X) {
		hsi_status |= SHIM_HSI_STATUS_STACKX;
	}

	tmp_alloc = AllocatePool(EFI_PAGE_SIZE);
	if (!tmp_alloc) {
		goto error;
	}

	addr = ((uintptr_t)tmp_alloc) & ~EFI_PAGE_MASK;
	efi_status = get_mem_attrs(addr, EFI_PAGE_MASK, &attrs);
	FreePool(tmp_alloc);
	if (EFI_ERROR(efi_status)) {
		goto error;
	}
	if (attrs & MEM_ATTR_X) {
		hsi_status |= SHIM_HSI_STATUS_HEAPX;
	}
	return;
	
error:
        /*
	 * In this case we can't actually tell anything, so assume
	 * and report the worst case scenario.
	 */
	hsi_status = SHIM_HSI_STATUS_HEAPX |
			     SHIM_HSI_STATUS_STACKX |
			     SHIM_HSI_STATUS_ROW;
}

@vathpela
Copy link
Member Author

Shouldn't this also explicitly expose if data sections of the image are executable or is that implied by heap is executable flag?

I'd rather 1) make that never happen and 2) make post-process-pe reject it. This should be reporting primarily about what firmware does.

@vathpela
Copy link
Member Author

May I suggest you could put error codes on the bottom of get_hsi_mem_info() ?

...

hsi_status = SHIM_HSI_STATUS_HEAPX |
SHIM_HSI_STATUS_STACKX |
SHIM_HSI_STATUS_ROW;
}

What's the improvement? Seems the same to me, but my habit is to put a series of 'undo' blocks in gotos at the end, but if it's just reporting an error, to do it at the first place it occurs.

@vathpela
Copy link
Member Author

Note that this is now on top of #726

@vathpela vathpela requested a review from jsetje February 19, 2025 20:31
@vathpela vathpela marked this pull request as ready for review February 19, 2025 20:32
@vathpela vathpela force-pushed the hsi branch 3 times, most recently from 145d1ac to 8b876bc Compare February 21, 2025 00:28
This changes get_mem_attrs() to return EFI_UNSUPPORTED if
LibLocateProtocol() does not return an error but does give us a NULL
pointer.

Signed-off-by: Peter Jones <pjones@redhat.com>
This adds a mok variable flag "MOK_VARIABLE_CONFIG_ONLY" to specify that
the data should be added to our UEFI config table, but shim should not
create a legacy UEFI variable.

Signed-off-by: Peter Jones <pjones@redhat.com>
This adds a member to the mok_state_variable struct to provide a
callback function for formatting external data.  It basically has
snprintf()-like semantics for filling the buffer, but without the actual
printf-like formatting bits.

Signed-off-by: Peter Jones <pjones@redhat.com>
Currently when you've added a variable and not correctly changed the
test cases to match, you get a message like:

	./test-mok-mirror
	test-mok-mirror: setting variable sort policy to MOCK_SORT_DESCENDING
	test-mok-mirror: setting delete policy to MOCK_VAR_DELETE_ATTR_ALLOW_ZERO
	running test_mok_mirror_with_enough_space
	test_mok_mirror_with_enough_space: passed
	running test_mok_mirror_setvar_out_of_resources
	check_config_table:232:mok.name[0] 72 != test.name[0] 0
	check_config_table:232:Assertion `mok_entry->name[0] == mock_entry->name[0]' failed.

This adds another two lines:

	test-mok-mirror: Failed on entry 4 mok.name:"HSIStatus" mock.name:""
	test-mok-mirror: Entry is missing in expected variable list.

Or:

	test-mok-mirror: Failed on entry 4 mok.name:"" mock.name:"HSIStatus"
	test-mok-mirror: Entry is missing in found variable list.

Which will usually tell you which variable you forgot to add that's
present in test data, or what's missing in the test data and present
in the expected data.

Signed-off-by: Peter Jones <pjones@redhat.com>
This moves the EFI Memory Attribute Protocol helper functions to their
own file, since they're not related to PE things.

Signed-off-by: Peter Jones <pjones@redhat.com>
hughsie asked me if I can make shim tell userland what kinds of accesses
are allowed to the heap, stack, and allocations on the running platform,
so that these could be reported up through fwupd's Host Security ID
program (see https://fwupd.github.io/libfwupdplugin/hsi.html ).

This adds a new config-only (i.e. not a UEFI variable) variable
generated during boot, "/sys/firmware/efi/mok-variables/HSIStatus",
which tells us those properties as well as if the EFI Memory Attribute
Protocol is present.

Signed-off-by: Peter Jones <pjones@redhat.com>
@vathpela vathpela merged commit 848667d into rhboot:main Feb 24, 2025
13 checks passed
@vathpela vathpela deleted the hsi branch February 24, 2025 20:24
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