| Message ID | 20181112094645.4879-3-fanc.fnst@cn.fujitsu.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | x86/boot/KASLR: Parse ACPI table and limit kaslr in immovable memory | expand |
On Mon, Nov 12, 2018 at 05:46:42PM +0800, Chao Fan wrote: > Imitate ACPI code to search RSDP pointer from memory. > Walk memory and check the signature until get the RSDP signature. > Based on acpi_tb_scan_memory_for_rsdp() and acpi_find_root_pointer(). > If didn't get RSDP from EFI table, will run this function. That's some very strange english. Please improve. > Used for later patch to dig out SRAT table and get the memory > information. And figure out the immovable memory regions > to avoid KASLR extracts kernel on movable memory, slove the ^^^^^^ Please introduce a spellchecker into your patch creation workflow. > conflict between KASLR and movable_node feature. Btw, this paragraph could be used for a CONFIG_ item you could define for your particular use case. Because right now you have funnies like: +#if (defined CONFIG_MEMORY_HOTREMOVE) && (defined CONFIG_RANDOMIZE_BASE) +vmlinux-objs-$(CONFIG_RANDOMIZE_BASE) += $(obj)/acpitb.o +#endif where CONFIG_RANDOMIZE_BASE is repeated for no good reason. But we'll see - need to get to the end of your patch series first. > Signed-off-by: Chao Fan <fanc.fnst@cn.fujitsu.com> > --- > arch/x86/boot/compressed/acpitb.c | 106 ++++++++++++++++++++++++++++++ > 1 file changed, 106 insertions(+) > > diff --git a/arch/x86/boot/compressed/acpitb.c b/arch/x86/boot/compressed/acpitb.c > index 56b54b0e0889..50fa65cf824d 100644 > --- a/arch/x86/boot/compressed/acpitb.c > +++ b/arch/x86/boot/compressed/acpitb.c > @@ -94,3 +94,109 @@ static void efi_get_rsdp_addr(acpi_physical_address *rsdp_addr) > } > #endif > } > + > +static u8 compute_checksum(u8 *buffer, u32 length) > +{ > + u8 sum = 0; > + u8 *end = buffer + length; > + > + while (buffer < end) > + sum = (u8)(sum + *(buffer++)); What's that cast for? Ah, this is the version in acpi_tb_checksum(). Well, I'd write this simply as: sum += *(buffer++); > + > + return sum; > +} > + > +/* > + * Used to search a block of memory for the RSDP signature. > + * Return Pointer to the RSDP if found, otherwise NULL. "Returns pointer... " > + * Based on acpi_tb_scan_memory_for_rsdp(). > + */ > +static u8 *scan_mem_for_rsdp(u8 *start, u32 length) > +{ > + struct acpi_table_rsdp *rsdp; > + u8 *end; > + u8 *rover; rover? > + > + end = start + length; > + > + /* Search from given start address for the requested length */ > + for (rover = start; rover < end; rover += ACPI_RSDP_SCAN_STEP) { > + /* > + * The RSDP signature and checksum must both be correct > + * Note: Sometimes there exists more than one RSDP in memory; > + * the valid RSDP has a valid checksum, all others have an > + * invalid checksum. > + */ > + rsdp = (struct acpi_table_rsdp *)rover; > + > + /* Nope, BAD Signature */ > + if (!ACPI_VALIDATE_RSDP_SIG(rsdp->signature)) > + continue; > + > + /* Check the standard checksum */ > + if (compute_checksum((u8 *) rsdp, ACPI_RSDP_CHECKSUM_LENGTH)) > + continue; > + > + /* Check extended checksum if table version >= 2 */ > + if ((rsdp->revision >= 2) && > + (compute_checksum((u8 *) rsdp, ACPI_RSDP_XCHECKSUM_LENGTH))) > + continue; > + > + /* Sig and checksum valid, we have found a real RSDP */ > + return rover; > + } > + return NULL; > +} > + > +/* > + * Used to search RSDP physical address. > + * Based on acpi_find_root_pointer(). Since only use physical address > + * in this period, so there is no need to do the memory map jobs. You mean: "All addresses used here are physical."? "memory map jobs"? Please be more careful when writing comments which are going to be read by other people. "jobs" means a lot of things and you don't want "jobs" in that context here. > + */ > +static void bios_get_rsdp_addr(acpi_physical_address *rsdp_addr) Same remark as before: the function is void and you're returning through its parameter. Make it return acpi_physical_address instead. > +{ > + struct acpi_table_rsdp *rsdp; > + u8 *table_ptr; > + u8 *mem_rover; rover? > + u32 address; > + > + /* > + * Get the location of the Extended BIOS Data Area (EBDA) > + * Since we use physical address directely, so It is "directly" - what about that spellchecker? > + * acpi_os_map_memory() and acpi_os_unmap_memory() are > + * not needed here. Why do you even need to say that here? > + */ > + table_ptr = (u8 *)ACPI_EBDA_PTR_LOCATION; > + *(u32 *)(void *)&address = *(u16 *)(void *)table_ptr; > + address <<= 4; > + table_ptr = (u8 *)address; arch/x86/boot/compressed/acpitb.c: In function ‘bios_get_rsdp_addr’: arch/x86/boot/compressed/acpitb.c:172:14: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast] table_ptr = (u8 *)address; ^ Also, that is some crazy casting here and I think you could use unsigned longs here for all the address arithmetic and cast to acpi_physical_address only at the end. > + > + /* > + * Search EBDA paragraphs (EBDA is required to be a minimum of > + * 1K length) > + */ > + if (address > 0x400) { > + mem_rover = scan_mem_for_rsdp(table_ptr, ACPI_EBDA_WINDOW_SIZE); > + Superfluous new line. > + if (mem_rover) { > + address += (u32)ACPI_PTR_DIFF(mem_rover, table_ptr); > + *rsdp_addr = (acpi_physical_address)address; > + return; > + } > + } > + > + table_ptr = (u8 *)ACPI_HI_RSDP_WINDOW_BASE; > + mem_rover = scan_mem_for_rsdp(table_ptr, ACPI_HI_RSDP_WINDOW_SIZE); > + > + /* > + * Search upper memory: 16-byte boundaries in E0000h-FFFFFh > + * Since we use physical address directely, so > + * acpi_os_map_memory() and acpi_os_unmap_memory() are > + * not needed here. > + */ And this comment needs to be repeated here because... ? > + if (mem_rover) { > + address = (u32)(ACPI_HI_RSDP_WINDOW_BASE + > + ACPI_PTR_DIFF(mem_rover, table_ptr)); > + *rsdp_addr = (acpi_physical_address)address; > + } > +} > --
On Mon, Nov 12, 2018 at 04:27:44PM +0100, Borislav Petkov wrote: >On Mon, Nov 12, 2018 at 05:46:42PM +0800, Chao Fan wrote: >> Imitate ACPI code to search RSDP pointer from memory. >> Walk memory and check the signature until get the RSDP signature. >> Based on acpi_tb_scan_memory_for_rsdp() and acpi_find_root_pointer(). >> If didn't get RSDP from EFI table, will run this function. > >That's some very strange english. Please improve. > >> Used for later patch to dig out SRAT table and get the memory >> information. And figure out the immovable memory regions >> to avoid KASLR extracts kernel on movable memory, slove the > ^^^^^^ > >Please introduce a spellchecker into your patch creation workflow. > Thanks. >> conflict between KASLR and movable_node feature. > >Btw, this paragraph could be used for a CONFIG_ item you could define >for your particular use case. Because right now you have funnies like: > >+#if (defined CONFIG_MEMORY_HOTREMOVE) && (defined CONFIG_RANDOMIZE_BASE) >+vmlinux-objs-$(CONFIG_RANDOMIZE_BASE) += $(obj)/acpitb.o >+#endif > >where CONFIG_RANDOMIZE_BASE is repeated for no good reason. > >But we'll see - need to get to the end of your patch series first. > >> Signed-off-by: Chao Fan <fanc.fnst@cn.fujitsu.com> >> --- >> arch/x86/boot/compressed/acpitb.c | 106 ++++++++++++++++++++++++++++++ >> 1 file changed, 106 insertions(+) >> >> diff --git a/arch/x86/boot/compressed/acpitb.c b/arch/x86/boot/compressed/acpitb.c >> index 56b54b0e0889..50fa65cf824d 100644 >> --- a/arch/x86/boot/compressed/acpitb.c >> +++ b/arch/x86/boot/compressed/acpitb.c >> @@ -94,3 +94,109 @@ static void efi_get_rsdp_addr(acpi_physical_address *rsdp_addr) >> } >> #endif >> } >> + >> +static u8 compute_checksum(u8 *buffer, u32 length) >> +{ >> + u8 sum = 0; >> + u8 *end = buffer + length; >> + >> + while (buffer < end) >> + sum = (u8)(sum + *(buffer++)); > >What's that cast for? > >Ah, this is the version in acpi_tb_checksum(). Well, I'd write this >simply as: > > sum += *(buffer++); Thanks for your suggestion. > >> + >> + return sum; >> +} >> + >> +/* >> + * Used to search a block of memory for the RSDP signature. >> + * Return Pointer to the RSDP if found, otherwise NULL. > > "Returns pointer... " > >> + * Based on acpi_tb_scan_memory_for_rsdp(). >> + */ >> +static u8 *scan_mem_for_rsdp(u8 *start, u32 length) >> +{ >> + struct acpi_table_rsdp *rsdp; >> + u8 *end; >> + u8 *rover; > >rover? > >> + >> + end = start + length; >> + >> + /* Search from given start address for the requested length */ >> + for (rover = start; rover < end; rover += ACPI_RSDP_SCAN_STEP) { The 'rover' was named as 'mem_rover', but the length of this line is too long. So shorten it as 'rever' so that they can keep in one line. >> + /* >> + * The RSDP signature and checksum must both be correct >> + * Note: Sometimes there exists more than one RSDP in memory; >> + * the valid RSDP has a valid checksum, all others have an >> + * invalid checksum. >> + */ >> + rsdp = (struct acpi_table_rsdp *)rover; >> + >> + /* Nope, BAD Signature */ >> + if (!ACPI_VALIDATE_RSDP_SIG(rsdp->signature)) >> + continue; >> + >> + /* Check the standard checksum */ >> + if (compute_checksum((u8 *) rsdp, ACPI_RSDP_CHECKSUM_LENGTH)) >> + continue; >> + >> + /* Check extended checksum if table version >= 2 */ >> + if ((rsdp->revision >= 2) && >> + (compute_checksum((u8 *) rsdp, ACPI_RSDP_XCHECKSUM_LENGTH))) >> + continue; >> + >> + /* Sig and checksum valid, we have found a real RSDP */ >> + return rover; >> + } >> + return NULL; >> +} >> + >> +/* >> + * Used to search RSDP physical address. >> + * Based on acpi_find_root_pointer(). Since only use physical address >> + * in this period, so there is no need to do the memory map jobs. > >You mean: "All addresses used here are physical."? > >"memory map jobs"? > >Please be more careful when writing comments which are going to be read >by other people. "jobs" means a lot of things and you don't want "jobs" >in that context here. OK. > >> + */ >> +static void bios_get_rsdp_addr(acpi_physical_address *rsdp_addr) > >Same remark as before: the function is void and you're returning through >its parameter. Make it return acpi_physical_address instead. > I will change all these functions. >> +{ >> + struct acpi_table_rsdp *rsdp; >> + u8 *table_ptr; >> + u8 *mem_rover; > >rover? This name came from ACPI driver code, acpi_find_root_pointer(). Used for the loop. If you have a better name, please tell me. > >> + u32 address; >> + >> + /* >> + * Get the location of the Extended BIOS Data Area (EBDA) >> + * Since we use physical address directely, so > >It is "directly" - what about that spellchecker? > >> + * acpi_os_map_memory() and acpi_os_unmap_memory() are >> + * not needed here. > >Why do you even need to say that here? I will try to improve all the comment. > >> + */ >> + table_ptr = (u8 *)ACPI_EBDA_PTR_LOCATION; >> + *(u32 *)(void *)&address = *(u16 *)(void *)table_ptr; >> + address <<= 4; >> + table_ptr = (u8 *)address; > >arch/x86/boot/compressed/acpitb.c: In function ‘bios_get_rsdp_addr’: >arch/x86/boot/compressed/acpitb.c:172:14: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast] > table_ptr = (u8 *)address; > ^ > >Also, that is some crazy casting here and I think you could use >unsigned longs here for all the address arithmetic and cast to >acpi_physical_address only at the end. That's a good suggestion. > >> + >> + /* >> + * Search EBDA paragraphs (EBDA is required to be a minimum of >> + * 1K length) >> + */ >> + if (address > 0x400) { >> + mem_rover = scan_mem_for_rsdp(table_ptr, ACPI_EBDA_WINDOW_SIZE); >> + > >Superfluous new line. > >> + if (mem_rover) { >> + address += (u32)ACPI_PTR_DIFF(mem_rover, table_ptr); >> + *rsdp_addr = (acpi_physical_address)address; >> + return; >> + } >> + } >> + >> + table_ptr = (u8 *)ACPI_HI_RSDP_WINDOW_BASE; >> + mem_rover = scan_mem_for_rsdp(table_ptr, ACPI_HI_RSDP_WINDOW_SIZE); >> + >> + /* >> + * Search upper memory: 16-byte boundaries in E0000h-FFFFFh >> + * Since we use physical address directely, so >> + * acpi_os_map_memory() and acpi_os_unmap_memory() are >> + * not needed here. >> + */ > >And this comment needs to be repeated here because... ? I will try to improve all the comment. Thanks, Chao Fan > >> + if (mem_rover) { >> + address = (u32)(ACPI_HI_RSDP_WINDOW_BASE + >> + ACPI_PTR_DIFF(mem_rover, table_ptr)); >> + *rsdp_addr = (acpi_physical_address)address; >> + } >> +} >> -- > >-- >Regards/Gruss, > Boris. > >Good mailing practices for 400: avoid top-posting and trim the reply. > >
On Tue, Nov 13, 2018 at 10:10:16AM +0800, Chao Fan wrote: > The 'rover' was named as 'mem_rover', but the length of this line is too > long. So shorten it as 'rever' so that they can keep in one line. I still have no clue what "rover" or "rever" means... > This name came from ACPI driver code, acpi_find_root_pointer(). > Used for the loop. If you have a better name, please tell me. I would if I knew what it meant. If you don't know either, then name it to something descriptive so that it is clear what it is when reading the code. Thx.
diff --git a/arch/x86/boot/compressed/acpitb.c b/arch/x86/boot/compressed/acpitb.c index 56b54b0e0889..50fa65cf824d 100644 --- a/arch/x86/boot/compressed/acpitb.c +++ b/arch/x86/boot/compressed/acpitb.c @@ -94,3 +94,109 @@ static void efi_get_rsdp_addr(acpi_physical_address *rsdp_addr) } #endif } + +static u8 compute_checksum(u8 *buffer, u32 length) +{ + u8 sum = 0; + u8 *end = buffer + length; + + while (buffer < end) + sum = (u8)(sum + *(buffer++)); + + return sum; +} + +/* + * Used to search a block of memory for the RSDP signature. + * Return Pointer to the RSDP if found, otherwise NULL. + * Based on acpi_tb_scan_memory_for_rsdp(). + */ +static u8 *scan_mem_for_rsdp(u8 *start, u32 length) +{ + struct acpi_table_rsdp *rsdp; + u8 *end; + u8 *rover; + + end = start + length; + + /* Search from given start address for the requested length */ + for (rover = start; rover < end; rover += ACPI_RSDP_SCAN_STEP) { + /* + * The RSDP signature and checksum must both be correct + * Note: Sometimes there exists more than one RSDP in memory; + * the valid RSDP has a valid checksum, all others have an + * invalid checksum. + */ + rsdp = (struct acpi_table_rsdp *)rover; + + /* Nope, BAD Signature */ + if (!ACPI_VALIDATE_RSDP_SIG(rsdp->signature)) + continue; + + /* Check the standard checksum */ + if (compute_checksum((u8 *) rsdp, ACPI_RSDP_CHECKSUM_LENGTH)) + continue; + + /* Check extended checksum if table version >= 2 */ + if ((rsdp->revision >= 2) && + (compute_checksum((u8 *) rsdp, ACPI_RSDP_XCHECKSUM_LENGTH))) + continue; + + /* Sig and checksum valid, we have found a real RSDP */ + return rover; + } + return NULL; +} + +/* + * Used to search RSDP physical address. + * Based on acpi_find_root_pointer(). Since only use physical address + * in this period, so there is no need to do the memory map jobs. + */ +static void bios_get_rsdp_addr(acpi_physical_address *rsdp_addr) +{ + struct acpi_table_rsdp *rsdp; + u8 *table_ptr; + u8 *mem_rover; + u32 address; + + /* + * Get the location of the Extended BIOS Data Area (EBDA) + * Since we use physical address directely, so + * acpi_os_map_memory() and acpi_os_unmap_memory() are + * not needed here. + */ + table_ptr = (u8 *)ACPI_EBDA_PTR_LOCATION; + *(u32 *)(void *)&address = *(u16 *)(void *)table_ptr; + address <<= 4; + table_ptr = (u8 *)address; + + /* + * Search EBDA paragraphs (EBDA is required to be a minimum of + * 1K length) + */ + if (address > 0x400) { + mem_rover = scan_mem_for_rsdp(table_ptr, ACPI_EBDA_WINDOW_SIZE); + + if (mem_rover) { + address += (u32)ACPI_PTR_DIFF(mem_rover, table_ptr); + *rsdp_addr = (acpi_physical_address)address; + return; + } + } + + table_ptr = (u8 *)ACPI_HI_RSDP_WINDOW_BASE; + mem_rover = scan_mem_for_rsdp(table_ptr, ACPI_HI_RSDP_WINDOW_SIZE); + + /* + * Search upper memory: 16-byte boundaries in E0000h-FFFFFh + * Since we use physical address directely, so + * acpi_os_map_memory() and acpi_os_unmap_memory() are + * not needed here. + */ + if (mem_rover) { + address = (u32)(ACPI_HI_RSDP_WINDOW_BASE + + ACPI_PTR_DIFF(mem_rover, table_ptr)); + *rsdp_addr = (acpi_physical_address)address; + } +}
Imitate ACPI code to search RSDP pointer from memory. Walk memory and check the signature until get the RSDP signature. Based on acpi_tb_scan_memory_for_rsdp() and acpi_find_root_pointer(). If didn't get RSDP from EFI table, will run this function. Used for later patch to dig out SRAT table and get the memory information. And figure out the immovable memory regions to avoid KASLR extracts kernel on movable memory, slove the conflict between KASLR and movable_node feature. Signed-off-by: Chao Fan <fanc.fnst@cn.fujitsu.com> --- arch/x86/boot/compressed/acpitb.c | 106 ++++++++++++++++++++++++++++++ 1 file changed, 106 insertions(+)