lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAE9FiQVQRAOtw7kiPJO3xszJx9uPFjgK82PjbvTyqzJgujdicw@mail.gmail.com> Date: Sun, 13 Jan 2013 21:37:08 -0800 From: Yinghai Lu <yinghai@...nel.org> To: Borislav Petkov <bp@...en8.de>, Yinghai Lu <yinghai@...nel.org>, Thomas Gleixner <tglx@...utronix.de>, Ingo Molnar <mingo@...e.hu>, "H. Peter Anvin" <hpa@...or.com>, "Eric W. Biederman" <ebiederm@...ssion.com>, Andrew Morton <akpm@...ux-foundation.org>, Jan Kiszka <jan.kiszka@....de>, Jason Wessel <jason.wessel@...driver.com>, linux-kernel@...r.kernel.org, Rob Landley <rob@...dley.net>, Matt Fleming <matt.fleming@...el.com>, Gokul Caushik <caushik1@...il.com>, Josh Triplett <josh@...htriplett.org>, Joe Millenbach <jmillenbach@...il.com> Subject: Re: [PATCH v7u1 22/31] x86, boot: add fields to support load bzImage and ramdisk above 4G On Sun, Jan 13, 2013 at 1:41 PM, Borislav Petkov <bp@...en8.de> wrote: > On Thu, Jan 03, 2013 at 04:48:42PM -0800, Yinghai Lu wrote: >> ext_ramdisk_image/size will record high 32bits for ramdisk info. >> >> xloadflags bit0 will be set if relocatable with 64bit. > > Let's describe that in more detail: > > "Bit 0 of xloadflags is set if we are both a 64-bit and a relocatable > kernel. In that case, it denotes that ramdisk can be loaded above 4Gb." > >> Let get_ramdisk_image/size to use ext_ramdisk_image/size to get >> right positon for ramdisk. >> >> bootloader will fill value to ext_ramdisk_image/size when it load >> ramdisk above 4G. >> >> Also bootloader will check if xloadflags bit0 is set to decicde if > > decide > >> it could load ramdisk high above 4G. >> >> sentinel is used to make sure kernel have ext_* valid values set > > The explanation of the sentinel field from "-v6" below should be > actually up here. We absolutely want to have it in the commit message > *and* in the code so that it is well documented why we've added it. > >> Update header version to 2.12. >> >> -v2: add ext_cmd_line_ptr for above 4G support. >> -v3: update to xloadflags from HPA. >> -v4: use fields from bootparam instead setup_header according to HPA. >> -v5: add checking for USE_EXT_BOOT_PARAMS >> -v6: use sentinel to check if ext_* are valid suggested by HPA. >> HPA said: >> 1. add a field in the uninitialized portion, call it "sentinel"; >> 2. make sure the byte position corresponding to the "sentinel" field is >> nonzero in the bzImage file; >> 3. if the kernel boots up and sentinel is nonzero, erase those fields >> that you identified as uninitialized; > > Question: if the bootloader sets ext_* properly, is it going to set > sentinel to 0 so that it can signal to the code further on that ext_* > are valid? old bootloaders have no idea of sentinel, but if they initialize boot_param properly that new sentinel will be 0 and new kernel will know. > > This is kinda missing from the mechanism of the sentinel and it should > be documented too. No, we should have too much duplicated info. > >> -v7: change to 0x1ef instead of 0x1f0, HPA said: >> it is quite plausible that someone may (fairly sanely) start the >> copy range at 0x1f0 instead of 0x1f1 > > Right, all those -vX notes are all important and should *definitely* be > at least in the commit message. No, I want to keep them in order to track the reviewing progress. > >> Signed-off-by: Yinghai Lu <yinghai@...nel.org> >> Cc: Rob Landley <rob@...dley.net> >> Cc: Matt Fleming <matt.fleming@...el.com> >> Cc: Gokul Caushik <caushik1@...il.com> >> Cc: Josh Triplett <josh@...htriplett.org> >> Cc: Joe Millenbach <jmillenbach@...il.com> >> --- >> Documentation/x86/boot.txt | 15 ++++++++++++++- >> Documentation/x86/zero-page.txt | 4 ++++ >> arch/x86/boot/compressed/cmdline.c | 2 ++ >> arch/x86/boot/compressed/misc.c | 12 ++++++++++++ >> arch/x86/boot/header.S | 12 ++++++++++-- >> arch/x86/boot/setup.ld | 7 +++++++ >> arch/x86/include/uapi/asm/bootparam.h | 13 ++++++++++--- >> arch/x86/kernel/head64.c | 2 ++ >> arch/x86/kernel/setup.c | 4 ++++ >> 9 files changed, 65 insertions(+), 6 deletions(-) >> >> diff --git a/Documentation/x86/boot.txt b/Documentation/x86/boot.txt >> index 406d82d..18ca9fb 100644 >> --- a/Documentation/x86/boot.txt >> +++ b/Documentation/x86/boot.txt >> @@ -57,6 +57,9 @@ Protocol 2.10: (Kernel 2.6.31) Added a protocol for relaxed alignment >> Protocol 2.11: (Kernel 3.6) Added a field for offset of EFI handover >> protocol entry point. >> >> +Protocol 2.12: (Kernel 3.9) Added three fields for loading bzImage and >> + ramdisk above 4G with 64bit in bootparam. > > change to: > > "Added three additional fields to bootparam used for loading bzImage and > ramdisk above 4Gb in 64-bit." ok > >> + >> **** MEMORY LAYOUT >> >> The traditional memory map for the kernel loader, used for Image or >> @@ -182,7 +185,7 @@ Offset Proto Name Meaning >> 0230/4 2.05+ kernel_alignment Physical addr alignment required for kernel >> 0234/1 2.05+ relocatable_kernel Whether kernel is relocatable or not >> 0235/1 2.10+ min_alignment Minimum alignment, as a power of two >> -0236/2 N/A pad3 Unused >> +0236/2 2.12+ xloadflags Boot protocol option flags >> 0238/4 2.06+ cmdline_size Maximum size of the kernel command line >> 023C/4 2.07+ hardware_subarch Hardware subarchitecture >> 0240/8 2.07+ hardware_subarch_data Subarchitecture-specific data >> @@ -582,6 +585,16 @@ Protocol: 2.10+ >> misaligned kernel. Therefore, a loader should typically try each >> power-of-two alignment from kernel_alignment down to this alignment. >> >> +Field name: xloadflags >> +Type: modify (obligatory) >> +Offset/size: 0x236/2 >> +Protocol: 2.12+ >> + >> + This field is a bitmask. >> + >> + Bit 0 (read): CAN_BE_LOADED_ABOVE_4G >> + - If 1, kernel/boot_params/cmdline/ramdisk can be above 4g, > > fullstop at the > end. ok. > >> + >> Field name: cmdline_size >> Type: read >> Offset/size: 0x238/4 >> diff --git a/Documentation/x86/zero-page.txt b/Documentation/x86/zero-page.txt >> index cf5437d..1140e59 100644 >> --- a/Documentation/x86/zero-page.txt >> +++ b/Documentation/x86/zero-page.txt >> @@ -19,6 +19,9 @@ Offset Proto Name Meaning >> 090/010 ALL hd1_info hd1 disk parameter, OBSOLETE!! >> 0A0/010 ALL sys_desc_table System description table (struct sys_desc_table) >> 0B0/010 ALL olpc_ofw_header OLPC's OpenFirmware CIF and friends >> +0C0/004 ALL ext_ramdisk_image ramdisk_image high 32bits >> +0C4/004 ALL ext_ramdisk_size ramdisk_size high 32bits >> +0C8/004 ALL ext_cmd_line_ptr cmd_line_ptr high 32bits >> 140/080 ALL edid_info Video mode setup (struct edid_info) >> 1C0/020 ALL efi_info EFI 32 information (struct efi_info) >> 1E0/004 ALL alk_mem_k Alternative mem check, in KB >> @@ -27,6 +30,7 @@ Offset Proto Name Meaning >> 1E9/001 ALL eddbuf_entries Number of entries in eddbuf (below) >> 1EA/001 ALL edd_mbr_sig_buf_entries Number of entries in edd_mbr_sig_buffer >> (below) >> +1EF/001 ALL sentinel 0: states _ext_* fields are valid >> 290/040 ALL edd_mbr_sig_buffer EDD MBR signatures >> 2D0/A00 ALL e820_map E820 memory map table >> (array of struct e820entry) >> diff --git a/arch/x86/boot/compressed/cmdline.c b/arch/x86/boot/compressed/cmdline.c >> index b4c913c..bffd73b 100644 >> --- a/arch/x86/boot/compressed/cmdline.c >> +++ b/arch/x86/boot/compressed/cmdline.c >> @@ -17,6 +17,8 @@ static unsigned long get_cmd_line_ptr(void) >> { >> unsigned long cmd_line_ptr = real_mode->hdr.cmd_line_ptr; >> >> + cmd_line_ptr |= (u64)real_mode->ext_cmd_line_ptr << 32; >> + >> return cmd_line_ptr; >> } > > On 32-bit, this unsigned long cmd_line_ptr is 4 bytes and the OR doesn't > have any effect on the final result. You probably want to do: yes, that is what we want to keep 32bit and 64bit unified. > > #ifdef CONFIG_64BIT > cmd_line_ptr |= (u64)real_mode->ext_cmd_line_ptr << 32; > #endif > > right? > > Or instead look at ->sentinel to know whether the ext_* fields are valid > or not, and save yourself the OR if not. no. that is whole point of sentinel, we don't need to check sentinel everywhere because ext_* are valid. > >> int cmdline_find_option(const char *option, char *buffer, int bufsize) >> diff --git a/arch/x86/boot/compressed/misc.c b/arch/x86/boot/compressed/misc.c >> index 88f7ff6..f714576 100644 >> --- a/arch/x86/boot/compressed/misc.c >> +++ b/arch/x86/boot/compressed/misc.c >> @@ -318,6 +318,16 @@ static void parse_elf(void *output) >> free(phdrs); >> } >> >> +static void sanitize_real_mode(struct boot_params *real_mode) >> +{ >> + if (real_mode->sentinel) { >> + /* ext_* fields in boot_params are not valid, clear them */ >> + real_mode->ext_ramdisk_image = 0; >> + real_mode->ext_ramdisk_size = 0; >> + real_mode->ext_cmd_line_ptr = 0; >> + } >> +} >> + >> asmlinkage void decompress_kernel(void *rmode, memptr heap, >> unsigned char *input_data, >> unsigned long input_len, >> @@ -325,6 +335,8 @@ asmlinkage void decompress_kernel(void *rmode, memptr heap, >> { >> real_mode = rmode; >> >> + sanitize_real_mode(real_mode); >> + >> if (real_mode->screen_info.orig_video_mode == 7) { >> vidmem = (char *) 0xb0000; >> vidport = 0x3b4; >> diff --git a/arch/x86/boot/header.S b/arch/x86/boot/header.S >> index 8c132a6..0d5790f 100644 >> --- a/arch/x86/boot/header.S >> +++ b/arch/x86/boot/header.S >> @@ -279,7 +279,7 @@ _start: >> # Part 2 of the header, from the old setup.S >> >> .ascii "HdrS" # header signature >> - .word 0x020b # header version number (>= 0x0105) >> + .word 0x020c # header version number (>= 0x0105) >> # or else old loadlin-1.5 will fail) >> .globl realmode_swtch >> realmode_swtch: .word 0, 0 # default_switch, SETUPSEG >> @@ -369,7 +369,15 @@ relocatable_kernel: .byte 1 >> relocatable_kernel: .byte 0 >> #endif >> min_alignment: .byte MIN_KERNEL_ALIGN_LG2 # minimum alignment >> -pad3: .word 0 >> + >> +xloadflags: >> +CAN_BE_LOADED_ABOVE_4G = 1 # If set, the kernel/boot_param/ >> + # ramdisk could be loaded above 4g >> +#if defined(CONFIG_X86_64) && defined(CONFIG_RELOCATABLE) >> + .word CAN_BE_LOADED_ABOVE_4G >> +#else >> + .word 0 >> +#endif >> >> cmdline_size: .long COMMAND_LINE_SIZE-1 #length of the command line, >> #added with boot protocol >> diff --git a/arch/x86/boot/setup.ld b/arch/x86/boot/setup.ld >> index 03c0683..9333d37 100644 >> --- a/arch/x86/boot/setup.ld >> +++ b/arch/x86/boot/setup.ld >> @@ -13,6 +13,13 @@ SECTIONS >> .bstext : { *(.bstext) } >> .bsdata : { *(.bsdata) } >> >> + /* sentinel: make sure if boot_params from bootloader is right */ > > This should say: > > /* > * The bootloader signals the validity of the three ext_* boot params with this. > */ no, bootloader does not signal that. old bootloaders have no idea of sentinel, but if they initialize boot_param properly that new sentinel will be 0 and new kernel will know. > >> + . = 495; >> + .sentinel : { >> + sentinel = .; >> + BYTE(0xff); >> + } >> + >> . = 497; >> .header : { *(.header) } >> .entrytext : { *(.entrytext) } >> diff --git a/arch/x86/include/uapi/asm/bootparam.h b/arch/x86/include/uapi/asm/bootparam.h >> index 92862cd..3d8ed8f 100644 >> --- a/arch/x86/include/uapi/asm/bootparam.h >> +++ b/arch/x86/include/uapi/asm/bootparam.h >> @@ -58,7 +58,9 @@ struct setup_header { >> __u32 initrd_addr_max; >> __u32 kernel_alignment; >> __u8 relocatable_kernel; >> - __u8 _pad2[3]; >> + __u8 min_alignment; > > Hehe, this is actually a minor bugfix because min_alignment was only > defined in header.S but was _pad2[0] in the struct definition. > >> + __u16 xloadflags; >> +#define CAN_BE_LOADED_ABOVE_4G (1<<0) >> __u32 cmdline_size; >> __u32 hardware_subarch; >> __u64 hardware_subarch_data; >> @@ -106,7 +108,10 @@ struct boot_params { >> __u8 hd1_info[16]; /* obsolete! */ /* 0x090 */ >> struct sys_desc_table sys_desc_table; /* 0x0a0 */ >> struct olpc_ofw_header olpc_ofw_header; /* 0x0b0 */ >> - __u8 _pad4[128]; /* 0x0c0 */ >> + __u32 ext_ramdisk_image; /* 0x0c0 */ >> + __u32 ext_ramdisk_size; /* 0x0c4 */ >> + __u32 ext_cmd_line_ptr; /* 0x0c8 */ >> + __u8 _pad4[116]; /* 0x0cc */ >> struct edid_info edid_info; /* 0x140 */ >> struct efi_info efi_info; /* 0x1c0 */ >> __u32 alt_mem_k; /* 0x1e0 */ >> @@ -115,7 +120,9 @@ struct boot_params { >> __u8 eddbuf_entries; /* 0x1e9 */ >> __u8 edd_mbr_sig_buf_entries; /* 0x1ea */ >> __u8 kbd_status; /* 0x1eb */ >> - __u8 _pad6[5]; /* 0x1ec */ >> + __u8 _pad5[3]; /* 0x1ec */ >> + __u8 sentinel; /* 0x1ef */ >> + __u8 _pad6[1]; /* 0x1f0 */ > > This needs the -v7 explanation from above as a comment here or somewhere > around here, for why we've chosen 0x1ef offset. no, there is no such comment for other fields there. Thanks Yinghai -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@...r.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists