[patch 0/2] goldfish: Prevent the trainwreck from doing harm

[patch 0/2] goldfish: Prevent the trainwreck from doing harm

[Thomas Gleixner]

The goldfish android emulator platform is a complete trainwreck which
causes havoc when enabled in Kconfig.

Aside of unconditionally registering a platform device with hardcoded
address ranges, it prevents the serial interrupt from being requested and
in case of a spurious interrupt the broken interrupt handler of the
pdev_bus driver goes into a hard to debug infinite loop.

Thanks to Gabriel and Borislav for providing patiently debug information.

No thanks to the folks @Intel who choked this $corporate trainwreck down
our throat and wasted the time of everyone who got bitten by this.

Thanks,

	tglx

[patch 1/2] x86/platform/goldfish: Prevent unconditional loading

[Thomas Gleixner]

[-- Attachment #1: x86-platform-goldfish--Prevent-unconditional-loading.patch --]
[-- Type: text/plain, Size: 2333 bytes --]

The goldfish platform code registers the platform device unconditionally
which causes havoc in several ways if the goldfish_pdev_bus driver is
enabled:

 - Access to the hardcoded physical memory region, which is either not
   available or contains stuff which is completely unrelated.

 - Prevents that the interrupt of the serial port can be requested

 - In case of a spurious interrupt it goes into a infinite loop in the
   interrupt handler of the pdev_bus driver (which needs to be fixed
   seperately).

Add a 'goldfish' command line option to make the registration opt-in when
the platform is compiled in.

I'm seriously grumpy about this engineering trainwreck, which has seven
SOBs from Intel developers for 50 lines of code. And none of them figured
out that this is broken. Impressive fail!

Fixes: ddd70cf93d78 ("goldfish: platform device for x86")
Reported-by: Gabriel C <nix.or.die@gmail.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: stable@vger.kernel.org
---
 Documentation/admin-guide/kernel-parameters.txt |    4 ++++
 arch/x86/platform/goldfish/goldfish.c           |   14 +++++++++++++-
 2 files changed, 17 insertions(+), 1 deletion(-)

--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -1192,6 +1192,10 @@
 			When zero, profiling data is discarded and associated
 			debugfs files are removed at module unload time.
 
+	goldfish	[X86] Enable the goldfish android emulator platform.
+			Don't use this when you are not running on the
+			android emulator
+
 	gpt		[EFI] Forces disk with valid GPT signature but
 			invalid Protective MBR to be treated as GPT. If the
 			primary GPT is corrupted, it enables the backup/alternate
--- a/arch/x86/platform/goldfish/goldfish.c
+++ b/arch/x86/platform/goldfish/goldfish.c
@@ -42,10 +42,22 @@ static struct resource goldfish_pdev_bus
 	}
 };
 
+static bool goldfish_enable __initdata;
+
+static int __init goldfish_setup(char *str)
+{
+	goldfish_enable = true;
+	return 0;
+}
+__setup("goldfish", goldfish_setup);
+
 static int __init goldfish_init(void)
 {
+	if (!goldfish_enable)
+		return -ENODEV;
+
 	platform_device_register_simple("goldfish_pdev_bus", -1,
-						goldfish_pdev_bus_resources, 2);
+					goldfish_pdev_bus_resources, 2);
 	return 0;
 }
 device_initcall(goldfish_init);

Re: [patch 1/2] x86/platform/goldfish: Prevent unconditional loading

[Peter Zijlstra]

On Wed, Feb 15, 2017 at 11:11:50AM +0100, Thomas Gleixner wrote:
> The goldfish platform code registers the platform device unconditionally
> which causes havoc in several ways if the goldfish_pdev_bus driver is
> enabled:
> 
>  - Access to the hardcoded physical memory region, which is either not
>    available or contains stuff which is completely unrelated.
> 
>  - Prevents that the interrupt of the serial port can be requested
> 
>  - In case of a spurious interrupt it goes into a infinite loop in the
>    interrupt handler of the pdev_bus driver (which needs to be fixed
>    seperately).
> 
> Add a 'goldfish' command line option to make the registration opt-in when
> the platform is compiled in.
> 
> I'm seriously grumpy about this engineering trainwreck, which has seven
> SOBs from Intel developers for 50 lines of code. And none of them figured
> out that this is broken. Impressive fail!
> 
> Fixes: ddd70cf93d78 ("goldfish: platform device for x86")
> Reported-by: Gabriel C <nix.or.die@gmail.com>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Cc: stable@vger.kernel.org

Just thinking, could we not simply delete this entire driver and use
x86-DT support to setup this platform?

Re: [patch 1/2] x86/platform/goldfish: Prevent unconditional loading

[Thomas Gleixner]

On Wed, 15 Feb 2017, Peter Zijlstra wrote:
> On Wed, Feb 15, 2017 at 11:11:50AM +0100, Thomas Gleixner wrote:
> > The goldfish platform code registers the platform device unconditionally
> > which causes havoc in several ways if the goldfish_pdev_bus driver is
> > enabled:
> > 
> >  - Access to the hardcoded physical memory region, which is either not
> >    available or contains stuff which is completely unrelated.
> > 
> >  - Prevents that the interrupt of the serial port can be requested
> > 
> >  - In case of a spurious interrupt it goes into a infinite loop in the
> >    interrupt handler of the pdev_bus driver (which needs to be fixed
> >    seperately).
> > 
> > Add a 'goldfish' command line option to make the registration opt-in when
> > the platform is compiled in.
> > 
> > I'm seriously grumpy about this engineering trainwreck, which has seven
> > SOBs from Intel developers for 50 lines of code. And none of them figured
> > out that this is broken. Impressive fail!
> > 
> > Fixes: ddd70cf93d78 ("goldfish: platform device for x86")
> > Reported-by: Gabriel C <nix.or.die@gmail.com>
> > Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> > Cc: stable@vger.kernel.org
> 
> Just thinking, could we not simply delete this entire driver and use
> x86-DT support to setup this platform?

That would be the proper solution and that's what ARM probably does already.

Thanks,

	tglx

Re: [patch 1/2] x86/platform/goldfish: Prevent unconditional loading

[Alan Cox]

> > > I'm seriously grumpy about this engineering trainwreck, which has
> > > seven
> > > SOBs from Intel developers for 50 lines of code. And none of them
> > > figured
> > > out that this is broken. Impressive fail!

It was discussed at the time, documented at the time. Unfortunately the
people who did the emulator didn't feel the urge to provide a way to
detect the platform was Goldfish.

Historically it also used its own custom device discovery scheme. Given
the limited use of older versions of Goldfish it might well make sense
to remove support for the older emulator versions.

Alan

Re: [patch 1/2] x86/platform/goldfish: Prevent unconditional loading

[Thomas Gleixner]

On Wed, 15 Feb 2017, Alan Cox wrote:

> > > > I'm seriously grumpy about this engineering trainwreck, which has
> > > > seven
> > > > SOBs from Intel developers for 50 lines of code. And none of them
> > > > figured
> > > > out that this is broken. Impressive fail!
> 
> It was discussed at the time, documented at the time.

I just have a hard time to find that documentation. It's definitely not in
the kernel source, unless you qualify the help text of CONFIG_GOLDFISH as
such:

     Enable support for the Goldfish virtual platform used primarily
     for Android development. Unless you are building for the Android
     Goldfish emulator say N here.

which does not help for randconfig and other builds and does not prevent
users from enabling it accidentaly. That all wouldn't be as bad if at least
the minimal provisioning of damage prevention would have been done.

> Unfortunately the people who did the emulator didn't feel the urge to
> provide a way to detect the platform was Goldfish.

Sure, and the people shoving it into the kernel didn't feel the urge to
enforce that.

> Historically it also used its own custom device discovery scheme. Given
> the limited use of older versions of Goldfish it might well make sense
> to remove support for the older emulator versions.

I'm all for it.

Thanks,

	tglx

[patch 2/2] goldfish: Sanitize the broken interrupt handler

[Thomas Gleixner]

[-- Attachment #1: goldfish--Sanitize-the-broken-interrupt-handler.patch --]
[-- Type: text/plain, Size: 1489 bytes --]

This interrupt handler is broken in several ways:

  - It loops forever when the op code is not decodeable

  - It never returns IRQ_HANDLED because the only way to exit the loop
    returns IRQ_NONE unconditionally.

The whole concept of this is broken. Creating devices in an interrupt
handler is beyond any point of sanity.

Make it at least behave halfways sane so accidental users do not have to
deal with a hard to debug lockup.

Fixes: e809c22b8fb028 ("goldfish: add the goldfish virtual bus")
Reported-by: Gabriel C <nix.or.die@gmail.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: stable@vger.kernel.org
---
 drivers/platform/goldfish/pdev_bus.c |   13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

--- a/drivers/platform/goldfish/pdev_bus.c
+++ b/drivers/platform/goldfish/pdev_bus.c
@@ -157,23 +157,26 @@ static int goldfish_new_pdev(void)
 static irqreturn_t goldfish_pdev_bus_interrupt(int irq, void *dev_id)
 {
 	irqreturn_t ret = IRQ_NONE;
+
 	while (1) {
 		u32 op = readl(pdev_bus_base + PDEV_BUS_OP);
-		switch (op) {
-		case PDEV_BUS_OP_DONE:
-			return IRQ_NONE;
 
+		switch (op) {
 		case PDEV_BUS_OP_REMOVE_DEV:
 			goldfish_pdev_remove();
+			ret = IRQ_HANDLED;
 			break;
 
 		case PDEV_BUS_OP_ADD_DEV:
 			goldfish_new_pdev();
+			ret = IRQ_HANDLED;
 			break;
+
+		case PDEV_BUS_OP_DONE:
+		default:
+			return ret;
 		}
-		ret = IRQ_HANDLED;
 	}
-	return ret;
 }
 
 static int goldfish_pdev_bus_probe(struct platform_device *pdev)

Re: [patch 0/2] goldfish: Prevent the trainwreck from doing harm

[Linus Torvalds]

Ack. I'm assuming Greg will pick this up, in the meantime I hope
anybody who has problems just disables the goldfish support.

Thanks,

               Linus

Re: [patch 0/2] goldfish: Prevent the trainwreck from doing harm

[Greg Kroah-Hartman]

On Wed, Feb 15, 2017 at 07:49:25AM -0800, Linus Torvalds wrote:
> Ack. I'm assuming Greg will pick this up, in the meantime I hope
> anybody who has problems just disables the goldfish support.

I've queued them up now for the next merge window, and they are tagged
for stable so will show up in 4.10.1.

thanks,

greg k-h