[kernel-hardening] Re: [PATCH 1/1] Ddrivers: hv: Turn off write permission on the hypercall page

[kernel-hardening] Re: [PATCH 1/1] Ddrivers: hv: Turn off write permission on the hypercall page

[Kees Cook]

On Wed, Feb 8, 2017 at 5:30 PM,  <kys@exchange.microsoft.com> wrote:
> From: K. Y. Srinivasan <kys@microsoft.com>
>
> The hypercall page only needs to be executable but currently it is setup to
> be writable as well. Fix the issue.
>
> Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> Cc: <stable@vger.kernel.org>
> ---
>  arch/x86/hyperv/hv_init.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/arch/x86/hyperv/hv_init.c b/arch/x86/hyperv/hv_init.c
> index c224b7d..db64baf 100644
> --- a/arch/x86/hyperv/hv_init.c
> +++ b/arch/x86/hyperv/hv_init.c
> @@ -125,7 +125,7 @@ void hyperv_init(void)
>         guest_id = generate_guest_id(0, LINUX_VERSION_CODE, 0);
>         wrmsrl(HV_X64_MSR_GUEST_OS_ID, guest_id);
>
> -       hypercall_pg  = __vmalloc(PAGE_SIZE, GFP_KERNEL, PAGE_KERNEL_EXEC);
> +       hypercall_pg  = __vmalloc(PAGE_SIZE, GFP_KERNEL, PAGE_KERNEL_RX);
>         if (hypercall_pg == NULL) {
>                 wrmsrl(HV_X64_MSR_GUEST_OS_ID, 0);
>                 return;
> --
> 1.7.1
>

Acked-by: Kees Cook <keescook@chromium.org>

This does beg the question: why is PAGE_KERNEL_EXEC defined at all?
The only permissions available should be _RO, _RW, or _RX... Current
users in arch/x86:

kernel/machine_kexec_32.c:      set_pte(pte, pfn_pte(paddr >>
PAGE_SHIFT, PAGE_KERNEL_EXEC));
kernel/machine_kexec_64.c:      set_pte(pte, pfn_pte(paddr >>
PAGE_SHIFT, PAGE_KERNEL_EXEC));
kernel/module.c:                                    PAGE_KERNEL_EXEC,
0, NUMA_NO_NODE,
kernel/tboot.c:         if (map_tboot_page(vaddr, start_pfn, PAGE_KERNEL_EXEC))
mm/init_32.c:                                   prot = PAGE_KERNEL_EXEC;
power/hibernate_32.c:                                   set_pte(pte,
pfn_pte(pfn, PAGE_KERNEL_EXEC));
xen/mmu.c:                      pte = pfn_pte(pfn, PAGE_KERNEL_EXEC);

and vmalloc_exec...

void *vmalloc_exec(unsigned long size)
{
        return __vmalloc_node(size, 1, GFP_KERNEL | __GFP_HIGHMEM,
PAGE_KERNEL_EXEC,
                              NUMA_NO_NODE, __builtin_return_address(0));
}

which is only ever used by module_alloc(), and the module code knows
how to make things RW, RO, and RX correctly. (Though on x86,
module_alloc() is overridden, but it still uses PAGE_KERNEL_EXEC.)

Seems like x86 should drop PAGE_KERNEL_EXEC, and vmalloc_exec should
use PAGE_KERNEL... ?

Or better yet, drop PAGE_KERNEL_EXEC globally and rename PAGE_KERNEL
to PAGE_KERNEL_RW.

-Kees

-- 
Kees Cook
Pixel Security

Re: [PATCH 1/1] Ddrivers: hv: Turn off write permission on the hypercall page

[Kees Cook]

On Wed, Feb 8, 2017 at 5:30 PM,  <kys@exchange.microsoft.com> wrote:
> From: K. Y. Srinivasan <kys@microsoft.com>
>
> The hypercall page only needs to be executable but currently it is setup to
> be writable as well. Fix the issue.
>
> Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> Cc: <stable@vger.kernel.org>
> ---
>  arch/x86/hyperv/hv_init.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/arch/x86/hyperv/hv_init.c b/arch/x86/hyperv/hv_init.c
> index c224b7d..db64baf 100644
> --- a/arch/x86/hyperv/hv_init.c
> +++ b/arch/x86/hyperv/hv_init.c
> @@ -125,7 +125,7 @@ void hyperv_init(void)
>         guest_id = generate_guest_id(0, LINUX_VERSION_CODE, 0);
>         wrmsrl(HV_X64_MSR_GUEST_OS_ID, guest_id);
>
> -       hypercall_pg  = __vmalloc(PAGE_SIZE, GFP_KERNEL, PAGE_KERNEL_EXEC);
> +       hypercall_pg  = __vmalloc(PAGE_SIZE, GFP_KERNEL, PAGE_KERNEL_RX);
>         if (hypercall_pg == NULL) {
>                 wrmsrl(HV_X64_MSR_GUEST_OS_ID, 0);
>                 return;
> --
> 1.7.1
>

Acked-by: Kees Cook <keescook@chromium.org>

This does beg the question: why is PAGE_KERNEL_EXEC defined at all?
The only permissions available should be _RO, _RW, or _RX... Current
users in arch/x86:

kernel/machine_kexec_32.c:      set_pte(pte, pfn_pte(paddr >>
PAGE_SHIFT, PAGE_KERNEL_EXEC));
kernel/machine_kexec_64.c:      set_pte(pte, pfn_pte(paddr >>
PAGE_SHIFT, PAGE_KERNEL_EXEC));
kernel/module.c:                                    PAGE_KERNEL_EXEC,
0, NUMA_NO_NODE,
kernel/tboot.c:         if (map_tboot_page(vaddr, start_pfn, PAGE_KERNEL_EXEC))
mm/init_32.c:                                   prot = PAGE_KERNEL_EXEC;
power/hibernate_32.c:                                   set_pte(pte,
pfn_pte(pfn, PAGE_KERNEL_EXEC));
xen/mmu.c:                      pte = pfn_pte(pfn, PAGE_KERNEL_EXEC);

and vmalloc_exec...

void *vmalloc_exec(unsigned long size)
{
        return __vmalloc_node(size, 1, GFP_KERNEL | __GFP_HIGHMEM,
PAGE_KERNEL_EXEC,
                              NUMA_NO_NODE, __builtin_return_address(0));
}

which is only ever used by module_alloc(), and the module code knows
how to make things RW, RO, and RX correctly. (Though on x86,
module_alloc() is overridden, but it still uses PAGE_KERNEL_EXEC.)

Seems like x86 should drop PAGE_KERNEL_EXEC, and vmalloc_exec should
use PAGE_KERNEL... ?

Or better yet, drop PAGE_KERNEL_EXEC globally and rename PAGE_KERNEL
to PAGE_KERNEL_RW.

-Kees

-- 
Kees Cook
Pixel Security

Re: [PATCH 1/1] Ddrivers: hv: Turn off write permission on the hypercall page

[Greg KH]

On Wed, Feb 08, 2017 at 06:30:56PM -0700, kys@exchange.microsoft.com wrote:
> From: K. Y. Srinivasan <kys@microsoft.com>
> 
> The hypercall page only needs to be executable but currently it is setup to
> be writable as well. Fix the issue.
> 
> Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> Cc: <stable@vger.kernel.org>

What type of bug is this fixing that it is needed to be backported?
Does this affect any current users?  How far back should it go?

thanks,

greg k-h

RE: [PATCH 1/1] Ddrivers: hv: Turn off write permission on the hypercall page

[KY Srinivasan]

> -----Original Message-----
> From: Greg KH [mailto:gregkh@linuxfoundation.org]
> Sent: Wednesday, February 8, 2017 10:55 PM
> To: KY Srinivasan <kys@microsoft.com>
> Cc: linux-kernel@vger.kernel.org; devel@linuxdriverproject.org;
> olaf@aepfle.de; apw@canonical.com; vkuznets@redhat.com;
> jasowang@redhat.com; leann.ogasawara@canonical.com;
> keescook@google.com; stephen@networkplumber.org; sds@tycho.nsa.gov;
> stable@vger.kernel.org
> Subject: Re: [PATCH 1/1] Ddrivers: hv: Turn off write permission on the
> hypercall page
> 
> On Wed, Feb 08, 2017 at 06:30:56PM -0700, kys@exchange.microsoft.com
> wrote:
> > From: K. Y. Srinivasan <kys@microsoft.com>
> >
> > The hypercall page only needs to be executable but currently it is setup to
> > be writable as well. Fix the issue.
> >
> > Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> > Cc: <stable@vger.kernel.org>
> 
> What type of bug is this fixing that it is needed to be backported?
> Does this affect any current users?  How far back should it go?

This issue has been there from day 1 and the mapping was setup after the
scan was done (for questionable mappings) and so we did not see it. Now, we
are setting up the hypercall page much earlier in the boot sequence and so
this issue was noticed. Once this is committed, I will send the patch for stable.
The main issue is that the page can be written and is executable and could be 
a vulnerability.

Thanks,

K. Y
> 
> thanks,
> 
> greg k-h

Re: [PATCH 1/1] Ddrivers: hv: Turn off write permission on the hypercall page

[Stephen Hemminger]

On Wed,  8 Feb 2017 18:30:56 -0700
kys@exchange.microsoft.com wrote:

> From: K. Y. Srinivasan <kys@microsoft.com>
> 
> The hypercall page only needs to be executable but currently it is setup to
> be writable as well. Fix the issue.
> 
> Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> Cc: <stable@vger.kernel.org>
> ---
>  arch/x86/hyperv/hv_init.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/x86/hyperv/hv_init.c b/arch/x86/hyperv/hv_init.c
> index c224b7d..db64baf 100644
> --- a/arch/x86/hyperv/hv_init.c
> +++ b/arch/x86/hyperv/hv_init.c
> @@ -125,7 +125,7 @@ void hyperv_init(void)
>  	guest_id = generate_guest_id(0, LINUX_VERSION_CODE, 0);
>  	wrmsrl(HV_X64_MSR_GUEST_OS_ID, guest_id);
>  
> -	hypercall_pg  = __vmalloc(PAGE_SIZE, GFP_KERNEL, PAGE_KERNEL_EXEC);
> +	hypercall_pg  = __vmalloc(PAGE_SIZE, GFP_KERNEL, PAGE_KERNEL_RX);
>  	if (hypercall_pg == NULL) {
>  		wrmsrl(HV_X64_MSR_GUEST_OS_ID, 0);

Verified that this fixes the original problem.
I don't see the warning anymore and the guest works normally.

Reported-by: Stephen Hemminger <stephen@networkplumber.org>
Tested-by: Stephen Hemminger <stephen@networkplumber.org>

Re: [PATCH 1/1] Ddrivers: hv: Turn off write permission on the hypercall page

[Greg KH]

On Wed, Feb 08, 2017 at 06:30:56PM -0700, kys@exchange.microsoft.com wrote:
> From: K. Y. Srinivasan <kys@microsoft.com>
> 
> The hypercall page only needs to be executable but currently it is setup to
> be writable as well. Fix the issue.
> 
> Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>

I had to hand-edit the subject: line, please be more careful :(

RE: [PATCH 1/1] Ddrivers: hv: Turn off write permission on the hypercall page

[KY Srinivasan]

> -----Original Message-----
> From: Greg KH [mailto:gregkh@linuxfoundation.org]
> Sent: Friday, February 10, 2017 6:48 AM
> To: KY Srinivasan <kys@microsoft.com>
> Cc: linux-kernel@vger.kernel.org; devel@linuxdriverproject.org;
> olaf@aepfle.de; apw@canonical.com; vkuznets@redhat.com;
> jasowang@redhat.com; leann.ogasawara@canonical.com;
> keescook@google.com; stephen@networkplumber.org; sds@tycho.nsa.gov;
> stable@vger.kernel.org
> Subject: Re: [PATCH 1/1] Ddrivers: hv: Turn off write permission on the
> hypercall page
> 
> On Wed, Feb 08, 2017 at 06:30:56PM -0700, kys@exchange.microsoft.com
> wrote:
> > From: K. Y. Srinivasan <kys@microsoft.com>
> >
> > The hypercall page only needs to be executable but currently it is setup to
> > be writable as well. Fix the issue.
> >
> > Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> 
> I had to hand-edit the subject: line, please be more careful :(

Thanks; will do.

K. Y