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

Add instructions on write-protecting bootblock #7

Merged
merged 1 commit into from
Sep 21, 2022

Conversation

SergiiDmytruk
Copy link
Member

No description provided.

Documentation/bootblock-protection.md Outdated Show resolved Hide resolved
(`printf "%#x\n" $(( 0xff7100 + 36544 - 1 ))`)

Thus we need to write-protect the smallest area that covers the range from
`0xff7100` to `0xffffbf` (both bounds are inclusive).

Choose a reason for hiding this comment

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

This is not correct, bootblock on x86 (except newest AMD boards that start from RAM) has to end at 0xffffffff. Offset from cbfstool print points to some kind of CBFS header that is not included in size reported by that same command. I haven't seen any other size of that metadata than 0x40, but I don't know if this is always true.

Choose a reason for hiding this comment

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

There is another possible issue: at 0xfffffffc there is a pointer to CBFS master header which is used to find CBFS. IIRC there were plans to change it to depend on FMAP instead, so maybe this is no longer an issue, but this could be the reason why 4.13 bootblock doesn't work with 4.17 rest of coreboot.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, that offset looked weird, but I forgot to check it. Doesn't look like the size is fixed ("offset" here is a field of component header):

/** This is a component header - every entry in the CBFS
    will have this header.

    This is how the component is arranged in the ROM:

    --------------   <- 0
    component header
    --------------   <- sizeof(struct component)
    component name
    --------------   <- offset
    data
    ...
    --------------   <- offset + len
*/

It's just that CBFS_ALIGNMENT is 64 which is why is most cases data will be offset by 64 bytes.


Looks like master CBFS header offset is still there on latest master (src/arch/x86/bootblock.ld) for the case of bootblock in CBFS:

	. = 0xfffffff0;
	_X86_RESET_VECTOR = .;
	.reset . : {
		*(.reset);
		. = _X86_RESET_VECTOR_FILLING;
		BYTE(0);
	}
	. = 0xfffffffc;
	.header_pointer . : {
		KEEP(*(.header_pointer));
	}
	_X86_RESET_VECTOR_FILLING = 15 - SIZEOF(.header_pointer);
	_ebootblock = .;

So it won't work if CBFS size changes. Size of image in 4.17 is 4 MiB vs. 255 KiB for 4.13, I didn't expect bootblock to contain that CBFS offset hard-coded and didn't try to make the size equal.

Choose a reason for hiding this comment

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

Regarding master header offset, I know it is still there but I haven't seen it being used in current code. Then again, I didn't dig too deep.

Copy link
Member Author

Choose a reason for hiding this comment

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

Even when ROM sizes match, offsets between bootblocks in 4.13 and 4.17 don't (12 byte difference).

Change-Id: Iaad1014fad5a60e78bef55c5f3aceaed782b66d6
Signed-off-by: Sergii Dmytruk <sergii.dmytruk@3mdeb.com>
Copy link

@macpijan macpijan left a comment

Choose a reason for hiding this comment

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

I went through this and it makes sense to me.

Based on that, I performed some testing on the x230 SPIs

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