linux-kernel - Re: [PATCH v3 05/11] s390/mm/fault: Handle guest-related program interrupts in KVM

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20241016100514.16801-B-hca@linux.ibm.com>
Date: Wed, 16 Oct 2024 12:05:14 +0200
From: Heiko Carstens <hca@...ux.ibm.com>
To: Claudio Imbrenda <imbrenda@...ux.ibm.com>
Cc: linux-kernel@...r.kernel.org, borntraeger@...ibm.com, nsg@...ux.ibm.com,
        nrb@...ux.ibm.com, frankja@...ux.ibm.com, seiden@...ux.ibm.com,
        agordeev@...ux.ibm.com, gor@...ux.ibm.com,
        gerald.schaefer@...ux.ibm.com, kvm@...r.kernel.org,
        linux-s390@...r.kernel.org
Subject: Re: [PATCH v3 05/11] s390/mm/fault: Handle guest-related program
 interrupts in KVM

On Tue, Oct 15, 2024 at 06:43:20PM +0200, Claudio Imbrenda wrote:
> Any program interrupt that happens in the host during the execution of
> a KVM guest will now short circuit the fault handler and return to KVM
> immediately. Guest fault handling (including pfault) will happen
> entirely inside KVM.
> 
> When sie64a() returns zero, current->thread.gmap_int_code will contain
> the program interrupt number that caused the exit, or zero if the exit
> was not caused by a host program interrupt.
> 
> KVM will now take care of handling all guest faults in vcpu_post_run().
> 
> Since gmap faults will not be visible by the rest of the kernel, remove
> GMAP_FAULT, the linux fault handlers for secure execution faults, the
> exception table entries for the sie instruction, the nop padding after
> the sie instruction, and all other references to guest faults from the
> s390 code.

...

> diff --git a/arch/s390/kernel/traps.c b/arch/s390/kernel/traps.c
...
> @@ -317,9 +318,23 @@ void noinstr __do_pgm_check(struct pt_regs *regs)
>  	struct lowcore *lc = get_lowcore();
>  	irqentry_state_t state;
>  	unsigned int trapnr;
> +	union teid teid = { .val = lc->trans_exc_code };
>  
>  	regs->int_code = lc->pgm_int_code;
> -	regs->int_parm_long = lc->trans_exc_code;
> +	regs->int_parm_long = teid.val;
> +
> +	/*
> +	 * In case of a guest fault, short-circuit the fault handler and return.
> +	 * This way the sie64a() function will return 0; fault address and
> +	 * other relevant bits are saved in current->thread.gmap_teid, and
> +	 * the fault number in current->thread.gmap_int_code. KVM will be
> +	 * able to use this information to handle the fault.
> +	 */
> +	if (test_pt_regs_flag(regs, PIF_GUEST_FAULT) && (teid.as == PSW_BITS_AS_PRIMARY)) {
> +		current->thread.gmap_teid.val = regs->int_parm_long;
> +		current->thread.gmap_int_code = regs->int_code & 0xffff;
> +		return;
> +	}

This check looks suboptimal to me for two reasons:

- if PIF_GUEST_FAULT is set it should never happen that the normal
  exception handling code is executed; it is clearly a bug if that
  would happen, and with the above check this may or may not be
  recognized with a kernel crash, if I'm not mistaken.

- __do_pgm_check() is executed for all program interruptions. This
  includes those interruptions which do not write a teid. Therefore
  the above check may do something unexpected depending on what teid a
  previous program interruption wrote. I think the teid.as check
  should be moved to kvm as well, and only be done for those cases
  where it is known that the teid contains a valid value.