| Message ID | 20121203200241.GG5906@thinkpad-t410 |
|---|---|
| State | Accepted |
| Headers | show |
On Mon, Dec 3, 2012 at 1:02 PM, Seth Forshee <seth.forshee@canonical.com> wrote: > On Thu, Oct 25, 2012 at 11:35:57AM -0600, Bjorn Helgaas wrote: >> On Thu, Aug 23, 2012 at 10:36 AM, Matthew Garrett <mjg@redhat.com> wrote: >> > V3 just fixes all the casting issues and incorporates David's change in >> > search ordering. >> >> I think there's still a section mismatch issue with these patches, so >> I haven't merged them yet. >> >> I rebased my pci/mjg-pci-roms-from-efi branch to v3.7-rc2, and if we >> get this issue fixed I'll put it in -next as v3.8 material. > > I still don't see this series in -next, so I take it the section > mismatch was never fixed? How about the following? That's right; nobody stepped up to fix the section mismatch. I'm happy to fold in your fix, especially if Matthew acks it. David, Eric, what about the kexec question? It looks to me like this wouldn't make things worse than they are today. If I understand correctly, today we don't use ROM data from EFI on either an initial boot or a kexec. After this patch, we could use EFI ROM data on the initial boot, but not after a kexec. So it's worse in the sense that the kexec case doesn't match the initial boot, but at least it's not something that used to work and is now broken. > From ece31852159a6b2cf9a059031638354e9817a6a6 Mon Sep 17 00:00:00 2001 > From: Seth Forshee <seth.forshee@canonical.com> > Date: Mon, 3 Dec 2012 13:55:50 -0600 > Subject: [PATCH] x86: Don't discard boot_params > > boot_params is now used at runtime on EFI systems to stash option ROMs > that aren't available after exiting boot services, so it can no longer > be marked __initdata. > > Signed-off-by: Seth Forshee <seth.forshee@canonical.com> > --- > arch/x86/kernel/setup.c | 4 ---- > 1 file changed, 4 deletions(-) > > diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c > index 468e98d..6e13035 100644 > --- a/arch/x86/kernel/setup.c > +++ b/arch/x86/kernel/setup.c > @@ -143,11 +143,7 @@ int default_check_phys_apicid_present(int phys_apicid) > } > #endif > > -#ifndef CONFIG_DEBUG_BOOT_PARAMS > -struct boot_params __initdata boot_params; > -#else > struct boot_params boot_params; > -#endif > > /* > * Machine setup.. > -- > 1.7.9.5 > -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Dec 05, 2012 at 01:09:25PM -0700, Bjorn Helgaas wrote: > That's right; nobody stepped up to fix the section mismatch. I'm > happy to fold in your fix, especially if Matthew acks it. Yes, sorry, I've been way behind on pretty much everything for the past few months. Please do add my Ack. > David, Eric, what about the kexec question? It looks to me like this > wouldn't make things worse than they are today. If I understand > correctly, today we don't use ROM data from EFI on either an initial > boot or a kexec. After this patch, we could use EFI ROM data on the > initial boot, but not after a kexec. So it's worse in the sense that > the kexec case doesn't match the initial boot, but at least it's not > something that used to work and is now broken. I think I'd agree here - it's not ideal, but it's no more broken than the current situation.
On Wed, 2012-12-05 at 13:09 -0700, Bjorn Helgaas wrote: > > David, Eric, what about the kexec question? It looks to me like this > wouldn't make things worse than they are today. If I understand > correctly, today we don't use ROM data from EFI on either an initial > boot or a kexec. After this patch, we could use EFI ROM data on the > initial boot, but not after a kexec. So it's worse in the sense that > the kexec case doesn't match the initial boot, but at least it's not > something that used to work and is now broken. Yeah, kexec under EFI doesn't work too well. I have a firmware running in qemu locally which will let you call SetVirtualAddressMap more than once, which is a step towards fixing it sanely. It got preempted, but I'll take another look at it shortly.
On Wed, Dec 5, 2012 at 1:22 PM, Matthew Garrett <mjg59@srcf.ucam.org> wrote: > On Wed, Dec 05, 2012 at 01:09:25PM -0700, Bjorn Helgaas wrote: > >> That's right; nobody stepped up to fix the section mismatch. I'm >> happy to fold in your fix, especially if Matthew acks it. > > Yes, sorry, I've been way behind on pretty much everything for the past > few months. Please do add my Ack. > >> David, Eric, what about the kexec question? It looks to me like this >> wouldn't make things worse than they are today. If I understand >> correctly, today we don't use ROM data from EFI on either an initial >> boot or a kexec. After this patch, we could use EFI ROM data on the >> initial boot, but not after a kexec. So it's worse in the sense that >> the kexec case doesn't match the initial boot, but at least it's not >> something that used to work and is now broken. > > I think I'd agree here - it's not ideal, but it's no more broken than > the current situation. OK, I applied Seth's fix, added his Tested-by, and put this series in my -next branch. I plan to merge it during the v3.8 merge window next week. Thanks! Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Dec 3, 2012 at 12:02 PM, Seth Forshee <seth.forshee@canonical.com> wrote: > On Thu, Oct 25, 2012 at 11:35:57AM -0600, Bjorn Helgaas wrote: >> On Thu, Aug 23, 2012 at 10:36 AM, Matthew Garrett <mjg@redhat.com> wrote: >> > V3 just fixes all the casting issues and incorporates David's change in >> > search ordering. >> >> I think there's still a section mismatch issue with these patches, so >> I haven't merged them yet. >> >> I rebased my pci/mjg-pci-roms-from-efi branch to v3.7-rc2, and if we >> get this issue fixed I'll put it in -next as v3.8 material. > > I still don't see this series in -next, so I take it the section > mismatch was never fixed? How about the following? > > > > From ece31852159a6b2cf9a059031638354e9817a6a6 Mon Sep 17 00:00:00 2001 > From: Seth Forshee <seth.forshee@canonical.com> > Date: Mon, 3 Dec 2012 13:55:50 -0600 > Subject: [PATCH] x86: Don't discard boot_params > > boot_params is now used at runtime on EFI systems to stash option ROMs > that aren't available after exiting boot services, so it can no longer > be marked __initdata. > > Signed-off-by: Seth Forshee <seth.forshee@canonical.com> > --- > arch/x86/kernel/setup.c | 4 ---- > 1 file changed, 4 deletions(-) > > diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c > index 468e98d..6e13035 100644 > --- a/arch/x86/kernel/setup.c > +++ b/arch/x86/kernel/setup.c > @@ -143,11 +143,7 @@ int default_check_phys_apicid_present(int phys_apicid) > } > #endif > > -#ifndef CONFIG_DEBUG_BOOT_PARAMS > -struct boot_params __initdata boot_params; > -#else > struct boot_params boot_params; > -#endif > > /* > * Machine setup.. No, that is not a right fix We should only cache pointer to setup_data. at the same time we should export setup_data into /sys, so kexec could append this pointer to command of second kernel, just like kexec append acpi_rsdp. That should address DavidW's concern. Yinghai -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Dec 05, 2012 at 04:15:56PM -0800, Yinghai Lu wrote: > at the same time we should export setup_data into /sys, so kexec could > append this pointer to command of > second kernel, just like kexec append acpi_rsdp. > That should address DavidW's concern. Why should the kernel export data to userspace just so that that data can be passed back into the kernel?
On 12/05/2012 04:18 PM, Matthew Garrett wrote: > On Wed, Dec 05, 2012 at 04:15:56PM -0800, Yinghai Lu wrote: > >> at the same time we should export setup_data into /sys, so kexec could >> append this pointer to command of >> second kernel, just like kexec append acpi_rsdp. >> That should address DavidW's concern. > > Why should the kernel export data to userspace just so that that data > can be passed back into the kernel? > Because it also needs to modify it. Right now kexec userspace synthesizes struct boot_params from scratch, and does so incorrectly to boot. I think we have setup_data exported via debugfs but IIRC we never got a strong enough use case for sysfs. -hpa -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Dec 5, 2012 at 4:18 PM, Matthew Garrett <mjg59@srcf.ucam.org> wrote: > On Wed, Dec 05, 2012 at 04:15:56PM -0800, Yinghai Lu wrote: > >> at the same time we should export setup_data into /sys, so kexec could >> append this pointer to command of >> second kernel, just like kexec append acpi_rsdp. >> That should address DavidW's concern. > > Why should the kernel export data to userspace just so that that data > can be passed back into the kernel? then how the kexeced get those info? first kernel already reserved those info as E820_RESERVED_KERN. and those info are for bootloader's of first kernel. -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Matthew Garrett <mjg59@srcf.ucam.org> writes: > On Wed, Dec 05, 2012 at 04:15:56PM -0800, Yinghai Lu wrote: > >> at the same time we should export setup_data into /sys, so kexec could >> append this pointer to command of >> second kernel, just like kexec append acpi_rsdp. >> That should address DavidW's concern. > > Why should the kernel export data to userspace just so that that data > can be passed back into the kernel? Because it is useful for more than just kexec and because that is the current architecture. Eric -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Dec 5, 2012 at 4:21 PM, H. Peter Anvin <hpa@zytor.com> wrote: > On 12/05/2012 04:18 PM, Matthew Garrett wrote: > Because it also needs to modify it. Right now kexec userspace > synthesizes struct boot_params from scratch, and does so incorrectly to > boot. I think we have setup_data exported via debugfs but IIRC we never > got a strong enough use case for sysfs. maybe we could try to export setup_data pointer only /sys, and that could be safe enough. -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12/05/2012 04:15 PM, Yinghai Lu wrote: > > -#ifndef CONFIG_DEBUG_BOOT_PARAMS > -struct boot_params __initdata boot_params; > -#else > struct boot_params boot_params; > -#endif > > No, that is not a right fix > > We should only cache pointer to setup_data. > > at the same time we should export setup_data into /sys, so kexec could > append this pointer to command of > second kernel, just like kexec append acpi_rsdp. > That should address DavidW's concern. > I don't see why that isn't the right fix. We copy the data into boot_params early in the boot; that *is* the official copy as far as the kernel is concerned. So this patch very much seems like The Right Thing. -hpa -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12/05/2012 04:30 PM, Yinghai Lu wrote: > On Wed, Dec 5, 2012 at 4:21 PM, H. Peter Anvin <hpa@zytor.com> wrote: >> On 12/05/2012 04:18 PM, Matthew Garrett wrote: > >> Because it also needs to modify it. Right now kexec userspace >> synthesizes struct boot_params from scratch, and does so incorrectly to >> boot. I think we have setup_data exported via debugfs but IIRC we never >> got a strong enough use case for sysfs. > > maybe we could try to export setup_data pointer only /sys, and that > could be safe enough. > I don't think there is anything security-sensitive about that information, at least not to root. I could be wrong, of course; I wouldn't mind security people telling me about that. -hpa -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Dec 5, 2012 at 4:36 PM, H. Peter Anvin <hpa@zytor.com> wrote: > On 12/05/2012 04:15 PM, Yinghai Lu wrote: >> > > I don't see why that isn't the right fix. We copy the data into > boot_params early in the boot; that *is* the official copy as far as the > kernel is concerned. > > So this patch very much seems like The Right Thing. it moves boot_params from __initdata to data. and just for using pointer to setup_data. should add setup_data pointer instead. so will not waste (4096 - 8) bytes. Yinghai -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Dec 5, 2012 at 4:51 PM, Yinghai Lu <yinghai@kernel.org> wrote: > On Wed, Dec 5, 2012 at 4:36 PM, H. Peter Anvin <hpa@zytor.com> wrote: >> On 12/05/2012 04:15 PM, Yinghai Lu wrote: >>> >> >> I don't see why that isn't the right fix. We copy the data into >> boot_params early in the boot; that *is* the official copy as far as the >> kernel is concerned. >> >> So this patch very much seems like The Right Thing. > > it moves boot_params from __initdata to data. should be from __initdata to bss > and just for using pointer to setup_data. > > should add setup_data pointer instead. so will not waste (4096 - 8) bytes. > > Yinghai -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12/05/2012 04:51 PM, Yinghai Lu wrote: > > it moves boot_params from __initdata to data. > and just for using pointer to setup_data. > > should add setup_data pointer instead. so will not waste (4096 - 8) bytes. > That is not the only bit. We already have covered how kexec could use the whole structure, and really, how big a deal is a page of cache-cold storage? -hpa -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
"H. Peter Anvin" <hpa@zytor.com> wrote: >I don't think there is anything security-sensitive about that >information, at least not to root. I could be wrong, of course; I >wouldn't mind security people telling me about that. I don't think there's anything at present, but we'll want to pass the hibernation encryption key from the bootloader to the kernel in the near future. setup_data seems like the easiest way to do that.
"H. Peter Anvin" <hpa@zytor.com> wrote: >Because it also needs to modify it. Right now kexec userspace >synthesizes struct boot_params from scratch, and does so incorrectly to >boot. I think we have setup_data exported via debugfs but IIRC we >never >got a strong enough use case for sysfs. Kexec userspace needs updating every time we add functionality to setup_data? That really doesn't sound ideal. If we want to be able to pass secrets between kernels then this needs to be done in kernel space, not userland.
On 12/05/2012 04:57 PM, Matthew Garrett wrote: > > > "H. Peter Anvin" <hpa@zytor.com> wrote: > >> I don't think there is anything security-sensitive about that >> information, at least not to root. I could be wrong, of course; I >> wouldn't mind security people telling me about that. > > I don't think there's anything at present, but we'll want to pass the hibernation encryption key from the bootloader to the kernel in the near future. setup_data seems like the easiest way to do that. > And that presumably would be something that cannot be exposed to root? If so we may want to use one of the bits in the setup_data type field as a security flag, perhaps... -hpa -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
"H. Peter Anvin" <hpa@zytor.com> wrote: >And that presumably would be something that cannot be exposed to root? >If so we may want to use one of the bits in the setup_data type field >as >a security flag, perhaps... Yeah, it needs to be hidden from root - but ideally we'd be passing it to the second kernel if we kexec. Alternative would be for it to be capability bounded to a trusted signed kexec binary if we implement Vivek's IMA-based approach.
On 12/05/2012 05:13 PM, Matthew Garrett wrote: > > > "H. Peter Anvin" <hpa@zytor.com> wrote: > >> And that presumably would be something that cannot be exposed to root? >> If so we may want to use one of the bits in the setup_data type field >> as >> a security flag, perhaps... > > Yeah, it needs to be hidden from root - but ideally we'd be passing it to the second kernel if we kexec. Alternative would be for it to be capability bounded to a trusted signed kexec binary if we implement Vivek's IMA-based approach. > Either way a security flag in the type field makes sense. -hpa
On Wed, Dec 05, 2012 at 05:21:44PM -0800, H. Peter Anvin wrote: > On 12/05/2012 05:13 PM, Matthew Garrett wrote: > >Yeah, it needs to be hidden from root - but ideally we'd be passing it to the second kernel if we kexec. Alternative would be for it to be capability bounded to a trusted signed kexec binary if we implement Vivek's IMA-based approach. > > > > Either way a security flag in the type field makes sense. I've no objection to that, although I'm not sure there's any real reason to expose an incomplete setup_data to userspace. Any scenario in which kexec can't read the full data is one where kexec won't be able to call sys_kexec() anyway.
On Wed, Dec 5, 2012 at 5:52 PM, Yinghai Lu <yinghai@kernel.org> wrote: > On Wed, Dec 5, 2012 at 4:51 PM, Yinghai Lu <yinghai@kernel.org> wrote: >> On Wed, Dec 5, 2012 at 4:36 PM, H. Peter Anvin <hpa@zytor.com> wrote: >>> On 12/05/2012 04:15 PM, Yinghai Lu wrote: >>>> >>> >>> I don't see why that isn't the right fix. We copy the data into >>> boot_params early in the boot; that *is* the official copy as far as the >>> kernel is concerned. >>> >>> So this patch very much seems like The Right Thing. >> >> it moves boot_params from __initdata to data. > > should be from __initdata to bss > >> and just for using pointer to setup_data. >> >> should add setup_data pointer instead. so will not waste (4096 - 8) bytes. I'm not following the whole discussion here, but my impression is that what's in my -next branch is acceptable (http://git.kernel.org/?p=linux/kernel/git/helgaas/pci.git;a=commitdiff;h=328949ff10f2e3fcb11472571294beed39488342) If not, please explain further and provide a patch to fix it. Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12/06/2012 10:19 AM, Bjorn Helgaas wrote: > On Wed, Dec 5, 2012 at 5:52 PM, Yinghai Lu <yinghai@kernel.org> wrote: >> On Wed, Dec 5, 2012 at 4:51 PM, Yinghai Lu <yinghai@kernel.org> wrote: >>> On Wed, Dec 5, 2012 at 4:36 PM, H. Peter Anvin <hpa@zytor.com> wrote: >>>> On 12/05/2012 04:15 PM, Yinghai Lu wrote: >>>>> >>>> >>>> I don't see why that isn't the right fix. We copy the data into >>>> boot_params early in the boot; that *is* the official copy as far as the >>>> kernel is concerned. >>>> >>>> So this patch very much seems like The Right Thing. >>> >>> it moves boot_params from __initdata to data. >> >> should be from __initdata to bss >> >>> and just for using pointer to setup_data. >>> >>> should add setup_data pointer instead. so will not waste (4096 - 8) bytes. > > I'm not following the whole discussion here, but my impression is that > what's in my -next branch is acceptable > (http://git.kernel.org/?p=linux/kernel/git/helgaas/pci.git;a=commitdiff;h=328949ff10f2e3fcb11472571294beed39488342) > > If not, please explain further and provide a patch to fix it. > NAK on this bit: + if (boot_params.hdr.version < 0x0209) + return 0; This field is kernel->bootloader documentation. If a nonmaching value somehow leaks into the kernel, the kernel could either panic("Bootloader written by moron") or it should clear some fields, but littering the kernel with these kinds of tests is just plain braindead. -hpa
On Thu, Dec 06, 2012 at 10:26:01AM -0800, H. Peter Anvin wrote: > NAK on this bit: > > + if (boot_params.hdr.version < 0x0209) > + return 0; > > This field is kernel->bootloader documentation. If a nonmaching > value somehow leaks into the kernel, the kernel could either > panic("Bootloader written by moron") or it should clear some fields, > but littering the kernel with these kinds of tests is just plain > braindead. Dropping that should be fine. Bjorn, would you prefer a patch from me to do that?
On Thu, Dec 6, 2012 at 10:54 AM, Matthew Garrett <mjg59@srcf.ucam.org> wrote: > On Thu, Dec 06, 2012 at 10:26:01AM -0800, H. Peter Anvin wrote: > >> NAK on this bit: >> >> + if (boot_params.hdr.version < 0x0209) >> + return 0; >> >> This field is kernel->bootloader documentation. If a nonmaching >> value somehow leaks into the kernel, the kernel could either >> panic("Bootloader written by moron") or it should clear some fields, >> but littering the kernel with these kinds of tests is just plain >> braindead. > > Dropping that should be fine. Bjorn, would you prefer a patch from me to > do that? the one : use pointer for setup_pci instead.
On Thu, Dec 6, 2012 at 11:54 AM, Matthew Garrett <mjg59@srcf.ucam.org> wrote: > On Thu, Dec 06, 2012 at 10:26:01AM -0800, H. Peter Anvin wrote: > >> NAK on this bit: >> >> + if (boot_params.hdr.version < 0x0209) >> + return 0; >> >> This field is kernel->bootloader documentation. If a nonmaching >> value somehow leaks into the kernel, the kernel could either >> panic("Bootloader written by moron") or it should clear some fields, >> but littering the kernel with these kinds of tests is just plain >> braindead. > > Dropping that should be fine. Bjorn, would you prefer a patch from me to > do that? I dropped that check and re-pushed my -next branch. Yinghai, I don't understand your pa_data_setup_pci.patch, but if somebody else acks it and we can come up with an intelligible changelog, I'll fold that in, too. -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c index 468e98d..6e13035 100644 --- a/arch/x86/kernel/setup.c +++ b/arch/x86/kernel/setup.c @@ -143,11 +143,7 @@ int default_check_phys_apicid_present(int phys_apicid) } #endif -#ifndef CONFIG_DEBUG_BOOT_PARAMS -struct boot_params __initdata boot_params; -#else struct boot_params boot_params; -#endif /* * Machine setup..