Re: [tip: x86/iopl] x86/iopl: Restrict iopl() permission scope

[Ingo Molnar]

* tip-bot2 for Thomas Gleixner <tip-bot2@xxxxxxxxxxxxx> wrote:

> The following commit has been merged into the x86/iopl branch of tip:
> 
> Commit-ID:     c8137ace56383688af911fea5934c71ad158135e
> Gitweb:        https://git.kernel.org/tip/c8137ace56383688af911fea5934c71ad158135e
> Author:        Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> AuthorDate:    Mon, 11 Nov 2019 23:03:28 +01:00
> Committer:     Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> CommitterDate: Sat, 16 Nov 2019 11:24:05 +01:00
> 
> x86/iopl: Restrict iopl() permission scope
> 
> The access to the full I/O port range can be also provided by the TSS I/O
> bitmap, but that would require to copy 8k of data on scheduling in the
> task. As shown with the sched out optimization TSS.io_bitmap_base can be
> used to switch the incoming task to a preallocated I/O bitmap which has all
> bits zero, i.e. allows access to all I/O ports.
> 
> Implementing this allows to provide an iopl() emulation mode which restricts
> the IOPL level 3 permissions to I/O port access but removes the STI/CLI
> permission which is coming with the hardware IOPL mechansim.
> 
> Provide a config option to switch IOPL to emulation mode, make it the
> default and while at it also provide an option to disable IOPL completely.
> 
> Signed-off-by: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> Acked-by: Andy Lutomirski <luto@xxxxxxxxxx>
> ---
>  arch/x86/Kconfig                        | 32 +++++++++-
>  arch/x86/include/asm/pgtable_32_types.h |  2 +-
>  arch/x86/include/asm/processor.h        | 28 ++++++--
>  arch/x86/kernel/cpu/common.c            |  5 +-
>  arch/x86/kernel/ioport.c                | 87 ++++++++++++++++--------
>  arch/x86/kernel/process.c               | 32 +++++----
>  6 files changed, 139 insertions(+), 47 deletions(-)

> --- a/arch/x86/include/asm/pgtable_32_types.h
> +++ b/arch/x86/include/asm/pgtable_32_types.h
> @@ -44,7 +44,7 @@ extern bool __vmalloc_start_set; /* set once high_memory is set */
>   * Define this here and validate with BUILD_BUG_ON() in pgtable_32.c
>   * to avoid include recursion hell
>   */
> -#define CPU_ENTRY_AREA_PAGES	(NR_CPUS * 40)
> +#define CPU_ENTRY_AREA_PAGES	(NR_CPUS * 41)

Note that this commit has two (fortunately harmless) bugs:

 - On 32-bit kernels the actual size of 'struct cpu_entry_area' was, 
   before this commit, 38 pages - while CPU_ENTRY_AREA_PAGES was 40, i.e.
   it's a pre-existing bug.

 - This commit increases cpu_entry_area by *TWO* pages (the new 
   ->mapall[] ioperm array is 8k large), while CPU_ENTRY_AREA_PAGES is 
   only increased by +1 page - but this is harmless, because we already 
   had an accidental 'reserve' of pages.

 - The resulting CPU_ENTRY_AREA_PAGES of 41 pages is still 1 page higher 
   than the true size of 40 pages.

The reason why these bugs remained undiscovered was that they are 
harmless (too many pages allocated), and the assert that was supposed to 
check these values was buggy.

So I'd suggest not rebasing this commit - especially since fixing the 
value will trigger the buggy assert, but wanted to give a heads-up.

I'll send the fix patch with more details to x86/urgent separately - and 
on merging x86/urgent to x86/iopl in -tip I made the merge accurate, to 
resolve the resulting semantic conflict.

Thanks,

	Ingo