video:uvesafb: Fix oops that uvesafb try to execute NX-protected page

PATCH In review Wang YanQing 4 participants 8 messages Mar 27, 2012 – Apr 10, 2012 Framebuffer layer

Share & tools

Wang YanQing

author

2012-03-27 10:01 UTC #0 lore raw

PATCH 1/1 video:uvesafb: Fix oops that uvesafb try to execute NX-protected page

Ok! I try to check pcibios_enabled first, but get some opposition by Alan Cox, but I want to make thing work and fix the oops, so I choice the simple way to check the (__supported_pte_mask & _PAGE_NX) instead of to check this variable plus pci kernel boot parameter, pci mmconfig works or not, and more, and more. It is not the best method, but it works and maybe all will feel happy.

Signed-off-by Wang YanQing <[email protected]>

---
 drivers/video/uvesafb.c |   11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

drivers/video/uvesafb.c +9 -2

index 260cca7..26e83d7 100644
@@ -815,8 +815,15 @@ static int __devinit uvesafb_vbe_init(struct fb_info *info)
 	par->pmi_setpal = pmi_setpal;
 	par->ypan = ypan;
 
-	if (par->pmi_setpal || par->ypan)
-		uvesafb_vbe_getpmi(task, par);
+	if (par->pmi_setpal || par->ypan) {
+		if (__supported_pte_mask & _PAGE_NX) {
+			par->pmi_setpal = par->ypan = 0;
+			printk(KERN_WARNING "uvesafb: NX protection is actively."
+				"We have better not to use the PMI.\n");
+		} else {
+			uvesafb_vbe_getpmi(task, par);
+		}
+	}
 #else
 	/* The protected mode interface is not available on non-x86. */
 	par->pmi_setpal = par->ypan = 0;

Signature · 1 line

1.7.9.2.315.g25a78

Alan Cox

2012-03-27 13:30 UTC #1 lore raw

Quoted 6 lines · On Tue, 27 Mar 2012 18:01:36 +0800 Wang YanQing <[email protected]> wrote:

Ok! I try to check pcibios_enabled first, but get some opposition by Alan Cox, but I want to make thing work and fix the oops, so I choice the simple way to check the (__supported_pte_mask & _PAGE_NX) instead of to check this variable plus pci kernel boot parameter, pci mmconfig works or not, and more, and more. It is not the best method, but it works and maybe all will feel happy.

Okay let me ask the obvious question - why is it not the best method ?

Apart from adding a helper in the includes for the arch code of

	static inline is_nx_enabled(void)
	{
		return !!(__supported_pte_mask & _PAGE_NX);
	}

is there anything else it lacks ?

Yes ideally we'd set the relevant ROM areas executable, but for a simple fix is there anything else that's a problem with it ?

Alan

Wang YanQing

author

2012-03-28 00:52 UTC #2 lore raw

Quoted 19 lines · Alan Cox

Quoted 6 lines · On Tue, 27 Mar 2012 18:01:36 +0800 Wang YanQing <[email protected]> wrote:
Ok! I try to check pcibios_enabled first, but get some opposition by Alan Cox, but I want to make thing work and fix the oops, so I choice the simple way to check the (__supported_pte_mask & _PAGE_NX) instead of to check this variable plus pci kernel boot parameter, pci mmconfig works or not, and more, and more. It is not the best method, but it works and maybe all will feel happy.
Okay let me ask the obvious question - why is it not the best method ?
Apart from adding a helper in the includes for the arch code of
	static inline is_nx_enabled(void)
	{
		return !!(__supported_pte_mask & _PAGE_NX);
	}

Quoted 4 lines

is there anything else it lacks ?
Yes ideally we'd set the relevant ROM areas executable, but for a simple fix is there anything else that's a problem with it ?

Ok! Maybe you had missed my previous reply
http://permalink.gmane.org/gmane.linux.kernel/1272433
It is not the best method, because the check is not enough.
I means when NX is actively, the pci bios is NX or not also depend on
the code path in pci_arch_init which will be influenced by the acpi on or off, pci kernel boot parameter, even kernel config like pci access method PCI_GOANY, PCI_GOMMCONFIG, or PCI_GODIRECT, but if I check the pcibios_enabled, all the above can be ignored.

if uvesafb use the PMI when PCI BIOS is X, it can get the better work efficience then use the redraw method as a fallback when do the panning.

Wang YanQing

author

2012-04-01 01:05 UTC #3 lore raw

Quoted 32 lines · Wang YanQing

Quoted 19 lines · Alan Cox
Quoted 6 lines · On Tue, 27 Mar 2012 18:01:36 +0800 Wang YanQing <[email protected]> wrote:
Ok! I try to check pcibios_enabled first, but get some opposition by Alan Cox, but I want to make thing work and fix the oops, so I choice the simple way to check the (__supported_pte_mask & _PAGE_NX) instead of to check this variable plus pci kernel boot parameter, pci mmconfig works or not, and more, and more. It is not the best method, but it works and maybe all will feel happy.
Okay let me ask the obvious question - why is it not the best method ?
Apart from adding a helper in the includes for the arch code of
	static inline is_nx_enabled(void)
	{
		return !!(__supported_pte_mask & _PAGE_NX);
	}
Quoted 4 lines
is there anything else it lacks ?
Yes ideally we'd set the relevant ROM areas executable, but for a simple fix is there anything else that's a problem with it ?
Ok! Maybe you had missed my previous reply
http://permalink.gmane.org/gmane.linux.kernel/1272433
It is not the best method, because the check is not enough.
I means when NX is actively, the pci bios is NX or not also depend on
the code path in pci_arch_init which will be influenced by the acpi on or off, pci kernel boot parameter, even kernel config like pci access method PCI_GOANY, PCI_GOMMCONFIG, or PCI_GODIRECT, but if I check the pcibios_enabled, all the above can be ignored.

Quoted 2 lines

if uvesafb use the PMI when PCI BIOS is X, it can get the better work efficience then use the redraw method as a fallback when do the panning.

Alan

I am just curious, I want to know what I describe above is right a little, or wrong about all the aspect.
thanks.

Wang YanQing

author

2012-04-10 06:49 UTC #4 lore raw

Quoted 33 lines · Wang YanQing

Quoted 32 lines · Wang YanQing
Quoted 19 lines · Alan Cox
Quoted 6 lines · On Tue, 27 Mar 2012 18:01:36 +0800 Wang YanQing <[email protected]> wrote:
Ok! I try to check pcibios_enabled first, but get some opposition by Alan Cox, but I want to make thing work and fix the oops, so I choice the simple way to check the (__supported_pte_mask & _PAGE_NX) instead of to check this variable plus pci kernel boot parameter, pci mmconfig works or not, and more, and more. It is not the best method, but it works and maybe all will feel happy.
Okay let me ask the obvious question - why is it not the best method ?
Apart from adding a helper in the includes for the arch code of
	static inline is_nx_enabled(void)
	{
		return !!(__supported_pte_mask & _PAGE_NX);
	}
Quoted 4 lines
is there anything else it lacks ?
Yes ideally we'd set the relevant ROM areas executable, but for a simple fix is there anything else that's a problem with it ?
Ok! Maybe you had missed my previous reply
http://permalink.gmane.org/gmane.linux.kernel/1272433
It is not the best method, because the check is not enough.
I means when NX is actively, the pci bios is NX or not also depend on
the code path in pci_arch_init which will be influenced by the acpi on or off, pci kernel boot parameter, even kernel config like pci access method PCI_GOANY, PCI_GOMMCONFIG, or PCI_GODIRECT, but if I check the pcibios_enabled, all the above can be ignored.

Quoted 8 lines

Quoted 2 lines
if uvesafb use the PMI when PCI BIOS is X, it can get the better work efficience then use the redraw method as a fallback when do the panning.
Alan
I am just curious, I want to know what I describe above is right a little, or wrong about all the aspect.
thanks.

Hi Alan, are you decided to not reply this any? But maybe we are still here to wait for your proposal to decide the final proper solution to fix the bug in kernel.

Thanks

Alan Cox

2012-04-10 14:26 UTC #5 lore raw

Quoted 1 line

Hi Alan, are you decided to not reply this any?

I'm not aware of anythign that needs adding to this. Any logic about whether PCI BIOS methods can be called belongs in the PCI layer, as does the necessary logic for marking ROMs executable if asked by a driver.

uvesafb isn't the only user of PCI methods that may be affected and the PCI layer is the place that should export the method to decide this.

That will also then work sanely cross platform - since in general on non x86 boxes the method can simply return "no"

Alan

Hein_Tibosch

2012-03-28 15:36 UTC #6 lore raw

In reply to Alan Cox

Quoted 10 lines · Alan Cox

Quoted 5 lines · On Tue, 27 Mar 2012 18:01:36 +0800 Wang YanQing <[email protected]> wrote:
Ok! I try to check pcibios_enabled first, but get some opposition by Alan Cox, but I want to make thing work and fix the oops, so I choice the simple way to check the (__supported_pte_mask & _PAGE_NX) instead of to check this variable plus pci kernel boot parameter, pci mmconfig works or not, and more, and more. It is not the best method, but it works and maybe all will feel happy.
Okay let me ask the obvious question - why is it not the best method ?

Quoted 8 lines

Apart from adding a helper in the includes for the arch code of
	static inline is_nx_enabled(void)
	{
		return !!(__supported_pte_mask & _PAGE_NX);
	}
is there anything else it lacks ?

maybe giving this inline function a return type ?

Florian Tobias Schandinat

2012-03-31 21:31 UTC #7 lore raw

In reply to Alan Cox

Quoted 9 lines · Alan Cox

Quoted 6 lines · On Tue, 27 Mar 2012 18:01:36 +0800 Wang YanQing <[email protected]> wrote:
Ok! I try to check pcibios_enabled first, but get some opposition by Alan Cox, but I want to make thing work and fix the oops, so I choice the simple way to check the (__supported_pte_mask & _PAGE_NX) instead of to check this variable plus pci kernel boot parameter, pci mmconfig works or not, and more, and more. It is not the best method, but it works and maybe all will feel happy.

Wang, could you please write a better commit message, at least you could include the oops that your patch fixes.

Quoted 6 lines

Apart from adding a helper in the includes for the arch code of
	static inline is_nx_enabled(void)
	{
		return !!(__supported_pte_mask & _PAGE_NX);
	}

As this check is already done in a few other places I think there should be a separate patch that adds this helper and ports all existing code to use it.

So, does anyone have any objection against me taking this patch as is?

Thanks,

Florian Tobias Schandinat