[PATCH] arm64: CONFIG_DEVPORT should not be used when PCI is being used

[PATCH] arm64: CONFIG_DEVPORT should not be used when PCI is being used

[Al Stone]

On arm64 systems, using /dev/port does not really make sense; this is
historically used for other architectures to access ISA IO ports, which
with any luck do not exist on arm64 platforms.  With the following snippet
of perl code (from Jeff Bastian <jbastian@redhat.com>), we can reliably
panic an arm64 system with PCI enabled:

	#!/usr/bin/perl -w
	# extracted from sensors-detect from lm_sensors
	# to reproduce kernel crash when probing the
	# Super-I/O ports
	use Fcntl qw(:DEFAULT :seek);
	sysopen(IOPORTS, "/dev/port", O_RDWR);
	binmode(IOPORTS);
	sysseek(IOPORTS, 0x2e, 0);
	syswrite(IOPORTS, pack("C", 0x0d), 1);

So, make sure CONFIG_DEVPORT cannot be set on arm64; it cannot really be
used and it allows us to crash a kernel from user space.

Signed-off-by: Al Stone <ahs3@redhat.com>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/char/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/char/Kconfig b/drivers/char/Kconfig
index b272397..c532f62 100644
--- a/drivers/char/Kconfig
+++ b/drivers/char/Kconfig
@@ -587,7 +587,7 @@ config TELCLOCK
 
 config DEVPORT
 	bool
-	depends on !M68K
+	depends on !M68K && !ARM64
 	depends on ISA || PCI
 	default y
 
-- 
1.8.3.1

Re: [PATCH] arm64: CONFIG_DEVPORT should not be used when PCI is being used

[Greg Kroah-Hartman]

On Wed, Apr 06, 2016 at 03:27:20PM -0600, Al Stone wrote:
> On arm64 systems, using /dev/port does not really make sense; this is
> historically used for other architectures to access ISA IO ports, which
> with any luck do not exist on arm64 platforms.  With the following snippet
> of perl code (from Jeff Bastian <jbastian@redhat.com>), we can reliably
> panic an arm64 system with PCI enabled:
> 
> 	#!/usr/bin/perl -w
> 	# extracted from sensors-detect from lm_sensors
> 	# to reproduce kernel crash when probing the
> 	# Super-I/O ports
> 	use Fcntl qw(:DEFAULT :seek);
> 	sysopen(IOPORTS, "/dev/port", O_RDWR);
> 	binmode(IOPORTS);
> 	sysseek(IOPORTS, 0x2e, 0);
> 	syswrite(IOPORTS, pack("C", 0x0d), 1);
> 
> So, make sure CONFIG_DEVPORT cannot be set on arm64; it cannot really be
> used and it allows us to crash a kernel from user space.
> 
> Signed-off-by: Al Stone <ahs3@redhat.com>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---
>  drivers/char/Kconfig | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/char/Kconfig b/drivers/char/Kconfig
> index b272397..c532f62 100644
> --- a/drivers/char/Kconfig
> +++ b/drivers/char/Kconfig
> @@ -587,7 +587,7 @@ config TELCLOCK
>  
>  config DEVPORT
>  	bool
> -	depends on !M68K
> +	depends on !M68K && !ARM64

Why not fix the real bug here, it's odd that only these two arches need
this disabled, don't you agree?

thanks,

greg k-h

Re: [PATCH] arm64: CONFIG_DEVPORT should not be used when PCI is being used

[Geert Uytterhoeven]

On Thu, Apr 7, 2016 at 2:18 AM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Wed, Apr 06, 2016 at 03:27:20PM -0600, Al Stone wrote:
>> On arm64 systems, using /dev/port does not really make sense; this is
>> historically used for other architectures to access ISA IO ports, which
>> with any luck do not exist on arm64 platforms.  With the following snippet
>> of perl code (from Jeff Bastian <jbastian@redhat.com>), we can reliably
>> panic an arm64 system with PCI enabled:
>>
>>       #!/usr/bin/perl -w
>>       # extracted from sensors-detect from lm_sensors
>>       # to reproduce kernel crash when probing the
>>       # Super-I/O ports
>>       use Fcntl qw(:DEFAULT :seek);
>>       sysopen(IOPORTS, "/dev/port", O_RDWR);
>>       binmode(IOPORTS);
>>       sysseek(IOPORTS, 0x2e, 0);
>>       syswrite(IOPORTS, pack("C", 0x0d), 1);

There are plenty of ways to crash a system as the root user...

>> So, make sure CONFIG_DEVPORT cannot be set on arm64; it cannot really be
>> used and it allows us to crash a kernel from user space.
>>
>> Signed-off-by: Al Stone <ahs3@redhat.com>
>> Cc: Arnd Bergmann <arnd@arndb.de>
>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> ---
>>  drivers/char/Kconfig | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/char/Kconfig b/drivers/char/Kconfig
>> index b272397..c532f62 100644
>> --- a/drivers/char/Kconfig
>> +++ b/drivers/char/Kconfig
>> @@ -587,7 +587,7 @@ config TELCLOCK
>>
>>  config DEVPORT
>>       bool
>> -     depends on !M68K
>> +     depends on !M68K && !ARM64
>
> Why not fix the real bug here, it's odd that only these two arches need
> this disabled, don't you agree?

In fact even the !M68K dependency is odd.
The logic seems to originate from commit 153dcc54df826d2f ("[PATCH] mem driver:
fix conditional on isa i/o support"), which accidentally changed an "||" into
an "&&".

Will send a patch later...

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] arm64: CONFIG_DEVPORT should not be used when PCI is being used

[Al Stone]

On 04/07/2016 01:26 AM, Geert Uytterhoeven wrote:
> On Thu, Apr 7, 2016 at 2:18 AM, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
>> On Wed, Apr 06, 2016 at 03:27:20PM -0600, Al Stone wrote:
>>> On arm64 systems, using /dev/port does not really make sense; this is
>>> historically used for other architectures to access ISA IO ports, which
>>> with any luck do not exist on arm64 platforms.  With the following snippet
>>> of perl code (from Jeff Bastian <jbastian@redhat.com>), we can reliably
>>> panic an arm64 system with PCI enabled:
>>>
>>>       #!/usr/bin/perl -w
>>>       # extracted from sensors-detect from lm_sensors
>>>       # to reproduce kernel crash when probing the
>>>       # Super-I/O ports
>>>       use Fcntl qw(:DEFAULT :seek);
>>>       sysopen(IOPORTS, "/dev/port", O_RDWR);
>>>       binmode(IOPORTS);
>>>       sysseek(IOPORTS, 0x2e, 0);
>>>       syswrite(IOPORTS, pack("C", 0x0d), 1);
> 
> There are plenty of ways to crash a system as the root user...

Of course.  This was just a new one.

>>> So, make sure CONFIG_DEVPORT cannot be set on arm64; it cannot really be
>>> used and it allows us to crash a kernel from user space.
>>>
>>> Signed-off-by: Al Stone <ahs3@redhat.com>
>>> Cc: Arnd Bergmann <arnd@arndb.de>
>>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>>> ---
>>>  drivers/char/Kconfig | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/char/Kconfig b/drivers/char/Kconfig
>>> index b272397..c532f62 100644
>>> --- a/drivers/char/Kconfig
>>> +++ b/drivers/char/Kconfig
>>> @@ -587,7 +587,7 @@ config TELCLOCK
>>>
>>>  config DEVPORT
>>>       bool
>>> -     depends on !M68K
>>> +     depends on !M68K && !ARM64
>>
>> Why not fix the real bug here, it's odd that only these two arches need
>> this disabled, don't you agree?

Agreed.  It does seem odd.  I'm not sure I understand which bug you're thinking
is the real one, though -- that DEVPORT should be disabled in all places that
don't have ISA or that arm64 needs to have /dev/port work properly?  Or perhaps
I missed something else entirely...

> In fact even the !M68K dependency is odd.
> The logic seems to originate from commit 153dcc54df826d2f ("[PATCH] mem driver:
> fix conditional on isa i/o support"), which accidentally changed an "||" into
> an "&&".
> 
> Will send a patch later...
> 
> Gr{oetje,eeting}s,
> 
>                         Geert

Aha.  I missed that bit.  Sorry about that.  So something like this instead:

config DEVPORT
	bool
	depends on ISA && PCI
	default y

That makes more sense.  Thanks.	

-- 
ciao,
al
-----------------------------------
Al Stone
Software Engineer
Red Hat, Inc.
ahs3@redhat.com
-----------------------------------

Re: [PATCH] arm64: CONFIG_DEVPORT should not be used when PCI is being used

[Arnd Bergmann]

On Thursday 07 April 2016, Al Stone wrote:
> >>>  config DEVPORT
> >>>       bool
> >>> -     depends on !M68K
> >>> +     depends on !M68K && !ARM64
> >>
> >> Why not fix the real bug here, it's odd that only these two arches need
> >> this disabled, don't you agree?
> 
> Agreed.  It does seem odd.  I'm not sure I understand which bug you're thinking
> is the real one, though -- that DEVPORT should be disabled in all places that
> don't have ISA or that arm64 needs to have /dev/port work properly?  Or perhaps
> I missed something else entirely...

We've had a similar problem recently with ISA drivers crashing when no PCI
host registers itself for the first 0x1000 I/O port addresses.

I think both the request_resource() function and /dev/ioport should be
changed to interface with the dynamic registration of I/O port ranges
so they only ever allow access on ports that are mapped into virtual memory.

	Arnd

Re: [PATCH] arm64: CONFIG_DEVPORT should not be used when PCI is being used

[Jon Masters]

On 04/07/2016 11:56 AM, Al Stone wrote:

> config DEVPORT
> 	bool
> 	depends on ISA && PCI
> 	default y
> 
> That makes more sense.  Thanks.

I think Itanium does IO ports but not ISA. Probably best to just turn on
IO ports on the three architectures that use them in that case.

Jon.

-- 
Computer Architect