Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1532673 > unrolled thread

[PATCHv4 00/10] CONFIG_DEBUG_VIRTUAL for arm64

Back to article view | Back to linux.kernel

Contents

  [PATCHv4 00/10] CONFIG_DEBUG_VIRTUAL for arm64 Laura Abbott <labbott@redhat.com> - 2016-11-29 20:00 +0100
    [PATCHv4 09/10] mm/usercopy: Switch to using lm_alias Laura Abbott <labbott@redhat.com> - 2016-11-29 20:00 +0100
      Re: [PATCHv4 09/10] mm/usercopy: Switch to using lm_alias Kees Cook <keescook@chromium.org> - 2016-11-29 20:40 +0100
        Re: [PATCHv4 09/10] mm/usercopy: Switch to using lm_alias Mark Rutland <mark.rutland@arm.com> - 2016-12-06 19:20 +0100
          Re: [PATCHv4 09/10] mm/usercopy: Switch to using lm_alias Kees Cook <keescook@chromium.org> - 2016-12-06 21:20 +0100
            Re: [PATCHv4 09/10] mm/usercopy: Switch to using lm_alias Mark Rutland <mark.rutland@arm.com> - 2016-12-07 15:00 +0100
      Re: [PATCHv4 09/10] mm/usercopy: Switch to using lm_alias Mark Rutland <mark.rutland@arm.com> - 2016-12-06 19:30 +0100
    [PATCHv4 10/10] arm64: Add support for CONFIG_DEBUG_VIRTUAL Laura Abbott <labbott@redhat.com> - 2016-11-29 20:00 +0100
      Re: [PATCHv4 10/10] arm64: Add support for CONFIG_DEBUG_VIRTUAL Mark Rutland <mark.rutland@arm.com> - 2016-12-06 20:00 +0100
    [PATCHv4 08/10] mm/kasan: Switch to using __pa_symbol and lm_alias Laura Abbott <labbott@redhat.com> - 2016-11-29 20:00 +0100
      Re: [PATCHv4 08/10] mm/kasan: Switch to using __pa_symbol and  lm_alias Laura Abbott <labbott@redhat.com> - 2016-12-01 20:20 +0100
      Re: [PATCHv4 08/10] mm/kasan: Switch to using __pa_symbol and  lm_alias Mark Rutland <mark.rutland@arm.com> - 2016-12-06 18:30 +0100
      Re: [PATCHv4 08/10] mm/kasan: Switch to using __pa_symbol and  lm_alias Mark Rutland <mark.rutland@arm.com> - 2016-12-06 18:50 +0100
      Re: [PATCHv4 08/10] mm/kasan: Switch to using __pa_symbol and  lm_alias Mark Rutland <mark.rutland@arm.com> - 2016-12-06 20:30 +0100
    [PATCHv4 06/10] xen: Switch to using __pa_symbol Laura Abbott <labbott@redhat.com> - 2016-11-29 20:00 +0100
      Re: [PATCHv4 06/10] xen: Switch to using __pa_symbol Boris Ostrovsky <boris.ostrovsky@oracle.com> - 2016-11-29 23:30 +0100
        Re: [PATCHv4 06/10] xen: Switch to using __pa_symbol Laura Abbott <labbott@redhat.com> - 2016-11-29 23:50 +0100
    [PATCH 0/3] ARM: Add support for CONFIG_DEBUG_VIRTUAL Florian Fainelli <f.fainelli@gmail.com> - 2016-12-06 21:00 +0100
      [PATCH 3/3] ARM: Add support for CONFIG_DEBUG_VIRTUAL Florian Fainelli <f.fainelli@gmail.com> - 2016-12-06 21:00 +0100
        Re: [PATCH 3/3] ARM: Add support for CONFIG_DEBUG_VIRTUAL Florian Fainelli <f.fainelli@gmail.com> - 2016-12-06 21:50 +0100
        Re: [PATCH 3/3] ARM: Add support for CONFIG_DEBUG_VIRTUAL Laura Abbott <labbott@redhat.com> - 2016-12-07 03:10 +0100
          Re: [PATCH 3/3] ARM: Add support for CONFIG_DEBUG_VIRTUAL Florian Fainelli <f.fainelli@gmail.com> - 2016-12-07 03:30 +0100
      [PATCH 2/3] ARM: Utilize __pa_symbol in lieu of __pa Florian Fainelli <f.fainelli@gmail.com> - 2016-12-06 21:00 +0100
      [PATCH 1/3] ARM: Define KERNEL_START and KERNEL_END Florian Fainelli <f.fainelli@gmail.com> - 2016-12-06 21:00 +0100
        Re: [PATCH 1/3] ARM: Define KERNEL_START and KERNEL_END Florian Fainelli <f.fainelli@gmail.com> - 2016-12-06 23:50 +0100
        RE: [PATCH 1/3] ARM: Define KERNEL_START and KERNEL_END Chris Brandt <Chris.Brandt@renesas.com> - 2016-12-06 23:50 +0100
        Re: [PATCH 1/3] ARM: Define KERNEL_START and KERNEL_END kbuild test robot <lkp@intel.com> - 2016-12-07 07:20 +0100

Page 1 of 2  [1] 2  Next page →

#1532673 — [PATCHv4 00/10] CONFIG_DEBUG_VIRTUAL for arm64

Hi,

This is v4 of the series to add CONFIG_DEBUG_VIRTUAL for arm64. This mostly
expanded on __pa_symbol conversion with a few new sites found. There's also
some reworking done to avoid calling __va too early. __va relies on having
memstart_addr set so very early code in early_fixmap_init and early KASAN
initialization can't just call __va(__Ipa_symbol(...)) to get the linear map
alias. I found this while testing with DEBUG_VM.

All of this could use probably use more testing under more configurations.
KVM, Xen, kexec, hibernate should all be tested.

Thanks,
Laura

Laura Abbott (10):
  lib/Kconfig.debug: Add ARCH_HAS_DEBUG_VIRTUAL
  mm/cma: Cleanup highmem check
  arm64: Move some macros under #ifndef __ASSEMBLY__
  arm64: Add cast for virt_to_pfn
  arm64: Use __pa_symbol for kernel symbols
  xen: Switch to using __pa_symbol
  kexec: Switch to __pa_symbol
  mm/kasan: Switch to using __pa_symbol and lm_alias
  mm/usercopy: Switch to using lm_alias
  arm64: Add support for CONFIG_DEBUG_VIRTUAL

 arch/arm64/Kconfig                        |  1 +
 arch/arm64/include/asm/kvm_mmu.h          |  4 +-
 arch/arm64/include/asm/memory.h           | 67 ++++++++++++++++++++++---------
 arch/arm64/include/asm/mmu_context.h      |  6 +--
 arch/arm64/include/asm/pgtable.h          |  2 +-
 arch/arm64/kernel/acpi_parking_protocol.c |  2 +-
 arch/arm64/kernel/cpu-reset.h             |  2 +-
 arch/arm64/kernel/cpufeature.c            |  2 +-
 arch/arm64/kernel/hibernate.c             | 13 +++---
 arch/arm64/kernel/insn.c                  |  2 +-
 arch/arm64/kernel/psci.c                  |  2 +-
 arch/arm64/kernel/setup.c                 |  8 ++--
 arch/arm64/kernel/smp_spin_table.c        |  2 +-
 arch/arm64/kernel/vdso.c                  |  4 +-
 arch/arm64/mm/Makefile                    |  2 +
 arch/arm64/mm/init.c                      | 11 ++---
 arch/arm64/mm/kasan_init.c                | 21 ++++++----
 arch/arm64/mm/mmu.c                       | 32 +++++++++------
 arch/arm64/mm/physaddr.c                  | 28 +++++++++++++
 arch/x86/Kconfig                          |  1 +
 drivers/firmware/psci.c                   |  2 +-
 drivers/xen/xenbus/xenbus_dev_backend.c   |  2 +-
 drivers/xen/xenfs/xenstored.c             |  2 +-
 include/linux/mm.h                        |  4 ++
 kernel/kexec_core.c                       |  2 +-
 lib/Kconfig.debug                         |  5 ++-
 mm/cma.c                                  | 15 +++----
 mm/kasan/kasan_init.c                     | 12 +++---
 mm/usercopy.c                             |  4 +-
 29 files changed, 167 insertions(+), 93 deletions(-)
 create mode 100644 arch/arm64/mm/physaddr.c

-- 
2.7.4

[toc] | [next] | [standalone]

#1532675 — [PATCHv4 09/10] mm/usercopy: Switch to using lm_alias

The usercopy checking code currently calls __va(__pa(...)) to check for
aliases on symbols. Switch to using lm_alias instead.

Signed-off-by: Laura Abbott <labbott@redhat.com>
---
Found when reviewing the kernel. Tested.
---
 mm/usercopy.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/mm/usercopy.c b/mm/usercopy.c
index 3c8da0a..8345299 100644
--- a/mm/usercopy.c
+++ b/mm/usercopy.c
@@ -108,13 +108,13 @@ static inline const char *check_kernel_text_object(const void *ptr,
 	 * __pa() is not just the reverse of __va(). This can be detected
 	 * and checked:
 	 */
-	textlow_linear = (unsigned long)__va(__pa(textlow));
+	textlow_linear = (unsigned long)lm_alias(textlow);
 	/* No different mapping: we're done. */
 	if (textlow_linear == textlow)
 		return NULL;
 
 	/* Check the secondary mapping... */
-	texthigh_linear = (unsigned long)__va(__pa(texthigh));
+	texthigh_linear = (unsigned long)lm_alias(texthigh);
 	if (overlaps(ptr, n, textlow_linear, texthigh_linear))
 		return "<linear kernel text>";
 
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1532713 — Re: [PATCHv4 09/10] mm/usercopy: Switch to using lm_alias

On Tue, Nov 29, 2016 at 10:55 AM, Laura Abbott <labbott@redhat.com> wrote:
>
> The usercopy checking code currently calls __va(__pa(...)) to check for
> aliases on symbols. Switch to using lm_alias instead.
>
> Signed-off-by: Laura Abbott <labbott@redhat.com>

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

I should probably add a corresponding alias test to lkdtm...

-Kees

> ---
> Found when reviewing the kernel. Tested.
> ---
>  mm/usercopy.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/mm/usercopy.c b/mm/usercopy.c
> index 3c8da0a..8345299 100644
> --- a/mm/usercopy.c
> +++ b/mm/usercopy.c
> @@ -108,13 +108,13 @@ static inline const char *check_kernel_text_object(const void *ptr,
>          * __pa() is not just the reverse of __va(). This can be detected
>          * and checked:
>          */
> -       textlow_linear = (unsigned long)__va(__pa(textlow));
> +       textlow_linear = (unsigned long)lm_alias(textlow);
>         /* No different mapping: we're done. */
>         if (textlow_linear == textlow)
>                 return NULL;
>
>         /* Check the secondary mapping... */
> -       texthigh_linear = (unsigned long)__va(__pa(texthigh));
> +       texthigh_linear = (unsigned long)lm_alias(texthigh);
>         if (overlaps(ptr, n, textlow_linear, texthigh_linear))
>                 return "<linear kernel text>";
>
> --
> 2.7.4
>



-- 
Kees Cook
Nexus Security

[toc] | [prev] | [next] | [standalone]

#1537184 — Re: [PATCHv4 09/10] mm/usercopy: Switch to using lm_alias

On Tue, Nov 29, 2016 at 11:39:44AM -0800, Kees Cook wrote:
> On Tue, Nov 29, 2016 at 10:55 AM, Laura Abbott <labbott@redhat.com> wrote:
> >
> > The usercopy checking code currently calls __va(__pa(...)) to check for
> > aliases on symbols. Switch to using lm_alias instead.
> >
> > Signed-off-by: Laura Abbott <labbott@redhat.com>
> 
> Acked-by: Kees Cook <keescook@chromium.org>
> 
> I should probably add a corresponding alias test to lkdtm...
> 
> -Kees

Something like the below?

It uses lm_alias(), so it depends on Laura's patches. We seem to do the
right thing, anyhow:

root@ribbensteg:/home/nanook# echo USERCOPY_KERNEL_ALIAS > /sys/kernel/debug/provoke-crash/DIRECT
[   44.493400] usercopy: kernel memory exposure attempt detected from ffff80000031a730 (<linear kernel text>) (4096 bytes)
[   44.504263] kernel BUG at mm/usercopy.c:75!

Thanks,
Mark.

---->8----
diff --git a/drivers/misc/lkdtm.h b/drivers/misc/lkdtm.h
index fdf954c..96d8d76 100644
--- a/drivers/misc/lkdtm.h
+++ b/drivers/misc/lkdtm.h
@@ -56,5 +56,6 @@
 void lkdtm_USERCOPY_STACK_FRAME_FROM(void);
 void lkdtm_USERCOPY_STACK_BEYOND(void);
 void lkdtm_USERCOPY_KERNEL(void);
+void lkdtm_USERCOPY_KERNEL_ALIAS(void);
 
 #endif
diff --git a/drivers/misc/lkdtm_core.c b/drivers/misc/lkdtm_core.c
index f9154b8..f6bc6d6 100644
--- a/drivers/misc/lkdtm_core.c
+++ b/drivers/misc/lkdtm_core.c
@@ -228,6 +228,7 @@ struct crashtype crashtypes[] = {
        CRASHTYPE(USERCOPY_STACK_FRAME_FROM),
        CRASHTYPE(USERCOPY_STACK_BEYOND),
        CRASHTYPE(USERCOPY_KERNEL),
+       CRASHTYPE(USERCOPY_KERNEL_ALIAS),
 };
 
 
diff --git a/drivers/misc/lkdtm_usercopy.c b/drivers/misc/lkdtm_usercopy.c
index 1dd6114..955f2dc 100644
--- a/drivers/misc/lkdtm_usercopy.c
+++ b/drivers/misc/lkdtm_usercopy.c
@@ -279,9 +279,16 @@ void lkdtm_USERCOPY_STACK_BEYOND(void)
        do_usercopy_stack(true, false);
 }
 
-void lkdtm_USERCOPY_KERNEL(void)
+static void do_usercopy_kernel(bool use_alias)
 {
        unsigned long user_addr;
+       const void *rodata = test_text;
+       void *text = vm_mmap;
+
+       if (use_alias) {
+               rodata = lm_alias(rodata);
+               text = lm_alias(text);
+       }
 
        user_addr = vm_mmap(NULL, 0, PAGE_SIZE,
                            PROT_READ | PROT_WRITE | PROT_EXEC,
@@ -292,14 +299,14 @@ void lkdtm_USERCOPY_KERNEL(void)
        }
 
        pr_info("attempting good copy_to_user from kernel rodata\n");
-       if (copy_to_user((void __user *)user_addr, test_text,
+       if (copy_to_user((void __user *)user_addr, rodata,
                         unconst + sizeof(test_text))) {
                pr_warn("copy_to_user failed unexpectedly?!\n");
                goto free_user;
        }
 
        pr_info("attempting bad copy_to_user from kernel text\n");
-       if (copy_to_user((void __user *)user_addr, vm_mmap,
+       if (copy_to_user((void __user *)user_addr, text,
                         unconst + PAGE_SIZE)) {
                pr_warn("copy_to_user failed, but lacked Oops\n");
                goto free_user;
@@ -309,6 +316,16 @@ void lkdtm_USERCOPY_KERNEL(void)
        vm_munmap(user_addr, PAGE_SIZE);
 }
 
+void lkdtm_USERCOPY_KERNEL(void)
+{
+       do_usercopy_kernel(false);
+}
+
+void lkdtm_USERCOPY_KERNEL_ALIAS(void)
+{
+       do_usercopy_kernel(true);
+}
+
 void __init lkdtm_usercopy_init(void)
 {
        /* Prepare cache that lacks SLAB_USERCOPY flag. */

[toc] | [prev] | [next] | [standalone]

#1537257 — Re: [PATCHv4 09/10] mm/usercopy: Switch to using lm_alias

On Tue, Dec 6, 2016 at 10:18 AM, Mark Rutland <mark.rutland@arm.com> wrote:
> On Tue, Nov 29, 2016 at 11:39:44AM -0800, Kees Cook wrote:
>> On Tue, Nov 29, 2016 at 10:55 AM, Laura Abbott <labbott@redhat.com> wrote:
>> >
>> > The usercopy checking code currently calls __va(__pa(...)) to check for
>> > aliases on symbols. Switch to using lm_alias instead.
>> >
>> > Signed-off-by: Laura Abbott <labbott@redhat.com>
>>
>> Acked-by: Kees Cook <keescook@chromium.org>
>>
>> I should probably add a corresponding alias test to lkdtm...
>>
>> -Kees
>
> Something like the below?
>
> It uses lm_alias(), so it depends on Laura's patches. We seem to do the
> right thing, anyhow:

Cool, this looks good. What happens on systems without an alias?

Laura, feel free to add this to your series:

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

-Kees

>
> root@ribbensteg:/home/nanook# echo USERCOPY_KERNEL_ALIAS > /sys/kernel/debug/provoke-crash/DIRECT
> [   44.493400] usercopy: kernel memory exposure attempt detected from ffff80000031a730 (<linear kernel text>) (4096 bytes)
> [   44.504263] kernel BUG at mm/usercopy.c:75!
>
> Thanks,
> Mark.
>
> ---->8----
> diff --git a/drivers/misc/lkdtm.h b/drivers/misc/lkdtm.h
> index fdf954c..96d8d76 100644
> --- a/drivers/misc/lkdtm.h
> +++ b/drivers/misc/lkdtm.h
> @@ -56,5 +56,6 @@
>  void lkdtm_USERCOPY_STACK_FRAME_FROM(void);
>  void lkdtm_USERCOPY_STACK_BEYOND(void);
>  void lkdtm_USERCOPY_KERNEL(void);
> +void lkdtm_USERCOPY_KERNEL_ALIAS(void);
>
>  #endif
> diff --git a/drivers/misc/lkdtm_core.c b/drivers/misc/lkdtm_core.c
> index f9154b8..f6bc6d6 100644
> --- a/drivers/misc/lkdtm_core.c
> +++ b/drivers/misc/lkdtm_core.c
> @@ -228,6 +228,7 @@ struct crashtype crashtypes[] = {
>         CRASHTYPE(USERCOPY_STACK_FRAME_FROM),
>         CRASHTYPE(USERCOPY_STACK_BEYOND),
>         CRASHTYPE(USERCOPY_KERNEL),
> +       CRASHTYPE(USERCOPY_KERNEL_ALIAS),
>  };
>
>
> diff --git a/drivers/misc/lkdtm_usercopy.c b/drivers/misc/lkdtm_usercopy.c
> index 1dd6114..955f2dc 100644
> --- a/drivers/misc/lkdtm_usercopy.c
> +++ b/drivers/misc/lkdtm_usercopy.c
> @@ -279,9 +279,16 @@ void lkdtm_USERCOPY_STACK_BEYOND(void)
>         do_usercopy_stack(true, false);
>  }
>
> -void lkdtm_USERCOPY_KERNEL(void)
> +static void do_usercopy_kernel(bool use_alias)
>  {
>         unsigned long user_addr;
> +       const void *rodata = test_text;
> +       void *text = vm_mmap;
> +
> +       if (use_alias) {
> +               rodata = lm_alias(rodata);
> +               text = lm_alias(text);
> +       }
>
>         user_addr = vm_mmap(NULL, 0, PAGE_SIZE,
>                             PROT_READ | PROT_WRITE | PROT_EXEC,
> @@ -292,14 +299,14 @@ void lkdtm_USERCOPY_KERNEL(void)
>         }
>
>         pr_info("attempting good copy_to_user from kernel rodata\n");
> -       if (copy_to_user((void __user *)user_addr, test_text,
> +       if (copy_to_user((void __user *)user_addr, rodata,
>                          unconst + sizeof(test_text))) {
>                 pr_warn("copy_to_user failed unexpectedly?!\n");
>                 goto free_user;
>         }
>
>         pr_info("attempting bad copy_to_user from kernel text\n");
> -       if (copy_to_user((void __user *)user_addr, vm_mmap,
> +       if (copy_to_user((void __user *)user_addr, text,
>                          unconst + PAGE_SIZE)) {
>                 pr_warn("copy_to_user failed, but lacked Oops\n");
>                 goto free_user;
> @@ -309,6 +316,16 @@ void lkdtm_USERCOPY_KERNEL(void)
>         vm_munmap(user_addr, PAGE_SIZE);
>  }
>
> +void lkdtm_USERCOPY_KERNEL(void)
> +{
> +       do_usercopy_kernel(false);
> +}
> +
> +void lkdtm_USERCOPY_KERNEL_ALIAS(void)
> +{
> +       do_usercopy_kernel(true);
> +}
> +
>  void __init lkdtm_usercopy_init(void)
>  {
>         /* Prepare cache that lacks SLAB_USERCOPY flag. */



-- 
Kees Cook
Nexus Security

[toc] | [prev] | [next] | [standalone]

#1537761 — Re: [PATCHv4 09/10] mm/usercopy: Switch to using lm_alias

On Tue, Dec 06, 2016 at 12:10:50PM -0800, Kees Cook wrote:
> On Tue, Dec 6, 2016 at 10:18 AM, Mark Rutland <mark.rutland@arm.com> wrote:
> > On Tue, Nov 29, 2016 at 11:39:44AM -0800, Kees Cook wrote:
> >> On Tue, Nov 29, 2016 at 10:55 AM, Laura Abbott <labbott@redhat.com> wrote:
> >> >
> >> > The usercopy checking code currently calls __va(__pa(...)) to check for
> >> > aliases on symbols. Switch to using lm_alias instead.
> >> >
> >> > Signed-off-by: Laura Abbott <labbott@redhat.com>
> >>
> >> Acked-by: Kees Cook <keescook@chromium.org>
> >>
> >> I should probably add a corresponding alias test to lkdtm...
> >>
> >> -Kees
> >
> > Something like the below?
> >
> > It uses lm_alias(), so it depends on Laura's patches. We seem to do the
> > right thing, anyhow:
> 
> Cool, this looks good. What happens on systems without an alias?

In that case, lm_alias() should be an identity function, and we'll just
hit the usual kernel address (i.e. it should be identical to
USERCOPY_KERNEL).

> Laura, feel free to add this to your series:
> 
> Acked-by: Kees Cook <keescook@chromium.org>

I'm happy with that, or I can resend this as a proper patch once the
rest is in.

Thanks,
Mark.

[toc] | [prev] | [next] | [standalone]

#1537196 — Re: [PATCHv4 09/10] mm/usercopy: Switch to using lm_alias

On Tue, Nov 29, 2016 at 10:55:28AM -0800, Laura Abbott wrote:
> 
> The usercopy checking code currently calls __va(__pa(...)) to check for
> aliases on symbols. Switch to using lm_alias instead.
> 
> Signed-off-by: Laura Abbott <labbott@redhat.com>

I've given this a go on Juno, which boots happily. LKDTM triggers as
expected when copying from the kernel text and its alias.

Reviewed-by: Mark Rutland <mark.rutland@arm.com>
Tested-by: Mark Rutland <mark.rutland@arm.com>

Thanks,
Mark.

> ---
> Found when reviewing the kernel. Tested.
> ---
>  mm/usercopy.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/mm/usercopy.c b/mm/usercopy.c
> index 3c8da0a..8345299 100644
> --- a/mm/usercopy.c
> +++ b/mm/usercopy.c
> @@ -108,13 +108,13 @@ static inline const char *check_kernel_text_object(const void *ptr,
>  	 * __pa() is not just the reverse of __va(). This can be detected
>  	 * and checked:
>  	 */
> -	textlow_linear = (unsigned long)__va(__pa(textlow));
> +	textlow_linear = (unsigned long)lm_alias(textlow);
>  	/* No different mapping: we're done. */
>  	if (textlow_linear == textlow)
>  		return NULL;
>  
>  	/* Check the secondary mapping... */
> -	texthigh_linear = (unsigned long)__va(__pa(texthigh));
> +	texthigh_linear = (unsigned long)lm_alias(texthigh);
>  	if (overlaps(ptr, n, textlow_linear, texthigh_linear))
>  		return "<linear kernel text>";
>  
> -- 
> 2.7.4
> 

[toc] | [prev] | [next] | [standalone]

#1532676 — [PATCHv4 10/10] arm64: Add support for CONFIG_DEBUG_VIRTUAL

x86 has an option CONFIG_DEBUG_VIRTUAL to do additional checks
on virt_to_phys calls. The goal is to catch users who are calling
virt_to_phys on non-linear addresses immediately. This inclues callers
using virt_to_phys on image addresses instead of __pa_symbol. As features
such as CONFIG_VMAP_STACK get enabled for arm64, this becomes increasingly
important. Add checks to catch bad virt_to_phys usage.

Signed-off-by: Laura Abbott <labbott@redhat.com>
---
v4: Refactored virt_to_phys macros for better reuse per suggestions.
---
 arch/arm64/Kconfig              |  1 +
 arch/arm64/include/asm/memory.h | 31 ++++++++++++++++++++++++++++---
 arch/arm64/mm/Makefile          |  2 ++
 arch/arm64/mm/physaddr.c        | 28 ++++++++++++++++++++++++++++
 4 files changed, 59 insertions(+), 3 deletions(-)
 create mode 100644 arch/arm64/mm/physaddr.c

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 969ef88..83b95bc 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -6,6 +6,7 @@ config ARM64
 	select ACPI_MCFG if ACPI
 	select ACPI_SPCR_TABLE if ACPI
 	select ARCH_CLOCKSOURCE_DATA
+	select ARCH_HAS_DEBUG_VIRTUAL
 	select ARCH_HAS_DEVMEM_IS_ALLOWED
 	select ARCH_HAS_ACPI_TABLE_UPGRADE if ACPI
 	select ARCH_HAS_ELF_RANDOMIZE
diff --git a/arch/arm64/include/asm/memory.h b/arch/arm64/include/asm/memory.h
index a219d3f..41ee96f 100644
--- a/arch/arm64/include/asm/memory.h
+++ b/arch/arm64/include/asm/memory.h
@@ -167,10 +167,33 @@ extern u64			kimage_voffset;
  * private definitions which should NOT be used outside memory.h
  * files.  Use virt_to_phys/phys_to_virt/__pa/__va instead.
  */
-#define __virt_to_phys(x) ({						\
+
+
+/*
+ * The linear kernel range starts in the middle of the virtual adddress
+ * space. Testing the top bit for the start of the region is a
+ * sufficient check.
+ */
+#define __is_lm_address(addr)	(!!((addr) & BIT(VA_BITS - 1)))
+
+#define __lm_to_phys(addr)	(((addr) & ~PAGE_OFFSET) + PHYS_OFFSET)
+#define __kimg_to_phys(addr)	((addr) - kimage_voffset)
+
+#define __virt_to_phys_nodebug(x) ({					\
 	phys_addr_t __x = (phys_addr_t)(x);				\
-	__x & BIT(VA_BITS - 1) ? (__x & ~PAGE_OFFSET) + PHYS_OFFSET :	\
-				 (__x - kimage_voffset); })
+	__is_lm_address(__x) ? __lm_to_phys(__x) :			\
+			       __kimg_to_phys(__x);			\
+})
+
+#define __pa_symbol_nodebug(x)	__kimg_to_phys((phys_addr_t)(x))
+
+#ifdef CONFIG_DEBUG_VIRTUAL
+extern phys_addr_t __virt_to_phys(unsigned long x);
+extern phys_addr_t __phys_addr_symbol(unsigned long x);
+#else
+#define __virt_to_phys(x)	__virt_to_phys_nodebug(x)
+#define __phys_addr_symbol(x)	__pa_symbol_nodebug(x)
+#endif
 
 #define __phys_to_virt(x)	((unsigned long)((x) - PHYS_OFFSET) | PAGE_OFFSET)
 #define __phys_to_kimg(x)	((unsigned long)((x) + kimage_voffset))
@@ -202,6 +225,8 @@ static inline void *phys_to_virt(phys_addr_t x)
  * Drivers should NOT use these either.
  */
 #define __pa(x)			__virt_to_phys((unsigned long)(x))
+#define __pa_symbol(x)		__phys_addr_symbol(RELOC_HIDE((unsigned long)(x), 0))
+#define __pa_nodebug(x)		__virt_to_phys_nodebug((unsigned long)(x))
 #define __va(x)			((void *)__phys_to_virt((phys_addr_t)(x)))
 #define pfn_to_kaddr(pfn)	__va((pfn) << PAGE_SHIFT)
 #define virt_to_pfn(x)      __phys_to_pfn(__virt_to_phys((unsigned long)(x)))
diff --git a/arch/arm64/mm/Makefile b/arch/arm64/mm/Makefile
index 54bb209..38d3811 100644
--- a/arch/arm64/mm/Makefile
+++ b/arch/arm64/mm/Makefile
@@ -5,6 +5,8 @@ obj-y				:= dma-mapping.o extable.o fault.o init.o \
 obj-$(CONFIG_HUGETLB_PAGE)	+= hugetlbpage.o
 obj-$(CONFIG_ARM64_PTDUMP)	+= dump.o
 obj-$(CONFIG_NUMA)		+= numa.o
+obj-$(CONFIG_DEBUG_VIRTUAL)	+= physaddr.o
+KASAN_SANITIZE_physaddr.o	+= n
 
 obj-$(CONFIG_KASAN)		+= kasan_init.o
 KASAN_SANITIZE_kasan_init.o	:= n
diff --git a/arch/arm64/mm/physaddr.c b/arch/arm64/mm/physaddr.c
new file mode 100644
index 0000000..6684f43
--- /dev/null
+++ b/arch/arm64/mm/physaddr.c
@@ -0,0 +1,28 @@
+#include <linux/bug.h>
+#include <linux/export.h>
+#include <linux/types.h>
+#include <linux/mmdebug.h>
+#include <linux/mm.h>
+
+#include <asm/memory.h>
+
+phys_addr_t __virt_to_phys(unsigned long x)
+{
+	WARN(!__is_lm_address(x),
+	     "virt_to_phys used for non-linear address :%pK\n", (void *)x);
+
+	return __virt_to_phys_nodebug(x);
+}
+EXPORT_SYMBOL(__virt_to_phys);
+
+phys_addr_t __phys_addr_symbol(unsigned long x)
+{
+	/*
+	 * This is bounds checking against the kernel image only.
+	 * __pa_symbol should only be used on kernel symbol addresses.
+	 */
+	VIRTUAL_BUG_ON(x < (unsigned long) KERNEL_START ||
+		       x > (unsigned long) KERNEL_END);
+	return __pa_symbol_nodebug(x);
+}
+EXPORT_SYMBOL(__phys_addr_symbol);
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1537210 — Re: [PATCHv4 10/10] arm64: Add support for CONFIG_DEBUG_VIRTUAL

On Tue, Nov 29, 2016 at 10:55:29AM -0800, Laura Abbott wrote:
> 
> +	WARN(!__is_lm_address(x),
> +	     "virt_to_phys used for non-linear address :%pK\n", (void *)x);

Nit: s/ :/: /

It might be worth adding %pS too; i.e.

	WARN(!__is_lm_address(x),
	     "virt_to_phys used for non-linear address: %pK (%pS)\n",
	     (void *)x, (void *)x);

... that way we might get a better idea before we have to resort to
grepping objdump output.

Other than that this looks good to me. This builds cleanly with and
without DEBUG_VIRTUAL enabled, and boots happily with DEBUG_VIRTUAL
disabled.

With both DEBUG_VIRTUAL and KASAN, I'm hitting a sea of warnings from
kasan_init at boot time, but I don't think that's a problem with this
patch as such, so FWIW:

Reviewed-by: Mark Rutland <mark.rutland@arm.com>
Tested-by: Mark Rutland <mark.rutland@arm.com>

Thanks,
Mark.

[toc] | [prev] | [next] | [standalone]

#1532678 — [PATCHv4 08/10] mm/kasan: Switch to using __pa_symbol and lm_alias

__pa_symbol is the correct API to find the physical address of symbols.
Switch to it to allow for debugging APIs to work correctly. Other
functions such as p*d_populate may call __pa internally. Ensure that the
address passed is in the linear region by calling lm_alias.

Signed-off-by: Laura Abbott <labbott@redhat.com>
---
Pointed out during review/testing of v3.
---
 mm/kasan/kasan_init.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/mm/kasan/kasan_init.c b/mm/kasan/kasan_init.c
index 3f9a41c..ff04721 100644
--- a/mm/kasan/kasan_init.c
+++ b/mm/kasan/kasan_init.c
@@ -49,7 +49,7 @@ static void __init zero_pte_populate(pmd_t *pmd, unsigned long addr,
 	pte_t *pte = pte_offset_kernel(pmd, addr);
 	pte_t zero_pte;
 
-	zero_pte = pfn_pte(PFN_DOWN(__pa(kasan_zero_page)), PAGE_KERNEL);
+	zero_pte = pfn_pte(PFN_DOWN(__pa_symbol(kasan_zero_page)), PAGE_KERNEL);
 	zero_pte = pte_wrprotect(zero_pte);
 
 	while (addr + PAGE_SIZE <= end) {
@@ -69,7 +69,7 @@ static void __init zero_pmd_populate(pud_t *pud, unsigned long addr,
 		next = pmd_addr_end(addr, end);
 
 		if (IS_ALIGNED(addr, PMD_SIZE) && end - addr >= PMD_SIZE) {
-			pmd_populate_kernel(&init_mm, pmd, kasan_zero_pte);
+			pmd_populate_kernel(&init_mm, pmd, lm_alias(kasan_zero_pte));
 			continue;
 		}
 
@@ -94,7 +94,7 @@ static void __init zero_pud_populate(pgd_t *pgd, unsigned long addr,
 
 			pud_populate(&init_mm, pud, kasan_zero_pmd);
 			pmd = pmd_offset(pud, addr);
-			pmd_populate_kernel(&init_mm, pmd, kasan_zero_pte);
+			pmd_populate_kernel(&init_mm, pmd, lm_alias(kasan_zero_pte));
 			continue;
 		}
 
@@ -135,11 +135,11 @@ void __init kasan_populate_zero_shadow(const void *shadow_start,
 			 * puds,pmds, so pgd_populate(), pud_populate()
 			 * is noops.
 			 */
-			pgd_populate(&init_mm, pgd, kasan_zero_pud);
+			pgd_populate(&init_mm, pgd, lm_alias(kasan_zero_pud));
 			pud = pud_offset(pgd, addr);
-			pud_populate(&init_mm, pud, kasan_zero_pmd);
+			pud_populate(&init_mm, pud, lm_alias(kasan_zero_pmd));
 			pmd = pmd_offset(pud, addr);
-			pmd_populate_kernel(&init_mm, pmd, kasan_zero_pte);
+			pmd_populate_kernel(&init_mm, pmd, lm_alias(kasan_zero_pte));
 			continue;
 		}
 
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1534389 — Re: [PATCHv4 08/10] mm/kasan: Switch to using __pa_symbol and lm_alias

On 12/01/2016 03:36 AM, Andrey Ryabinin wrote:
> On 11/29/2016 09:55 PM, Laura Abbott wrote:
>> __pa_symbol is the correct API to find the physical address of symbols.
>> Switch to it to allow for debugging APIs to work correctly.
> 
> But __pa() is correct for symbols. I see how __pa_symbol() might be a little
> faster than __pa(), but there is nothing wrong in using __pa() on symbols.
> 
>> Other
>> functions such as p*d_populate may call __pa internally. Ensure that the
>> address passed is in the linear region by calling lm_alias.
> 
> Why it should be linear mapping address? __pa() translates kernel image address just fine.
> This lm_alias() only obfuscates source code. Generated code is probably worse too.
> 
> 

This is part of adding CONFIG_DEBUG_VIRTUAL for arm64. We want to
differentiate between __pa and __pa_symbol to enforce stronger
virtual checks and have __pa only be for linear map addresses.

Thanks,
Laura

[toc] | [prev] | [next] | [standalone]

#1537148 — Re: [PATCHv4 08/10] mm/kasan: Switch to using __pa_symbol and lm_alias

On Thu, Dec 01, 2016 at 02:36:05PM +0300, Andrey Ryabinin wrote:
> On 11/29/2016 09:55 PM, Laura Abbott wrote:
> > __pa_symbol is the correct API to find the physical address of symbols.
> > Switch to it to allow for debugging APIs to work correctly.
> 
> But __pa() is correct for symbols. I see how __pa_symbol() might be a little
> faster than __pa(), but there is nothing wrong in using __pa() on symbols.

While it's true today that __pa() works on symbols, this is for
pragmatic reasons (allowing existing code to work until it is all
cleaned up), and __pa_symbol() is the correct API to use.

Relying on this means that __pa() can't be optimised for the (vastly
common) case of translating linear map addresses.

Consistent use of __pa_symbol() will allow for subsequent optimisation
of __pa() in the common case, in adition to being necessary for arm64's
DEBUG_VIRTUAL.

> > Other functions such as p*d_populate may call __pa internally.
> > Ensure that the address passed is in the linear region by calling
> > lm_alias.
> 
> Why it should be linear mapping address? __pa() translates kernel
> image address just fine.

As above, while that's true today, but is something that we wish to
change. 

> Generated code is probably worse too.

Even if that is the case, given this is code run once at boot on a debug
build, I think it's outweighed by the gain from DEBUG_VIRTUAL, and as a
step towards optimising __pa().

Thanks,
Mark.

[toc] | [prev] | [next] | [standalone]

#1537164 — Re: [PATCHv4 08/10] mm/kasan: Switch to using __pa_symbol and lm_alias

On Tue, Nov 29, 2016 at 10:55:27AM -0800, Laura Abbott wrote:
> __pa_symbol is the correct API to find the physical address of symbols.
> Switch to it to allow for debugging APIs to work correctly. Other
> functions such as p*d_populate may call __pa internally. Ensure that the
> address passed is in the linear region by calling lm_alias.

I've given this a go on Juno with CONFIG_KASAN_INLINE enabled, and
everything seems happy.

We'll need an include of <linux/mm.h> as that appears to be missing. I
guess we're getting lucky with transitive includes. Otherwise this looks
good to me.

With that fixed up:

Reviewed-by: Mark Rutland <mark.rutland@arm.com>
Tested-by: Mark Rutland <mark.rutland@arm.com>

Thanks,
Mark.

> Signed-off-by: Laura Abbott <labbott@redhat.com>
> ---
> Pointed out during review/testing of v3.
> ---
>  mm/kasan/kasan_init.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/mm/kasan/kasan_init.c b/mm/kasan/kasan_init.c
> index 3f9a41c..ff04721 100644
> --- a/mm/kasan/kasan_init.c
> +++ b/mm/kasan/kasan_init.c
> @@ -49,7 +49,7 @@ static void __init zero_pte_populate(pmd_t *pmd, unsigned long addr,
>  	pte_t *pte = pte_offset_kernel(pmd, addr);
>  	pte_t zero_pte;
>  
> -	zero_pte = pfn_pte(PFN_DOWN(__pa(kasan_zero_page)), PAGE_KERNEL);
> +	zero_pte = pfn_pte(PFN_DOWN(__pa_symbol(kasan_zero_page)), PAGE_KERNEL);
>  	zero_pte = pte_wrprotect(zero_pte);
>  
>  	while (addr + PAGE_SIZE <= end) {
> @@ -69,7 +69,7 @@ static void __init zero_pmd_populate(pud_t *pud, unsigned long addr,
>  		next = pmd_addr_end(addr, end);
>  
>  		if (IS_ALIGNED(addr, PMD_SIZE) && end - addr >= PMD_SIZE) {
> -			pmd_populate_kernel(&init_mm, pmd, kasan_zero_pte);
> +			pmd_populate_kernel(&init_mm, pmd, lm_alias(kasan_zero_pte));
>  			continue;
>  		}
>  
> @@ -94,7 +94,7 @@ static void __init zero_pud_populate(pgd_t *pgd, unsigned long addr,
>  
>  			pud_populate(&init_mm, pud, kasan_zero_pmd);
>  			pmd = pmd_offset(pud, addr);
> -			pmd_populate_kernel(&init_mm, pmd, kasan_zero_pte);
> +			pmd_populate_kernel(&init_mm, pmd, lm_alias(kasan_zero_pte));
>  			continue;
>  		}
>  
> @@ -135,11 +135,11 @@ void __init kasan_populate_zero_shadow(const void *shadow_start,
>  			 * puds,pmds, so pgd_populate(), pud_populate()
>  			 * is noops.
>  			 */
> -			pgd_populate(&init_mm, pgd, kasan_zero_pud);
> +			pgd_populate(&init_mm, pgd, lm_alias(kasan_zero_pud));
>  			pud = pud_offset(pgd, addr);
> -			pud_populate(&init_mm, pud, kasan_zero_pmd);
> +			pud_populate(&init_mm, pud, lm_alias(kasan_zero_pmd));
>  			pmd = pmd_offset(pud, addr);
> -			pmd_populate_kernel(&init_mm, pmd, kasan_zero_pte);
> +			pmd_populate_kernel(&init_mm, pmd, lm_alias(kasan_zero_pte));
>  			continue;
>  		}
>  
> -- 
> 2.7.4
> 

[toc] | [prev] | [next] | [standalone]

#1537233 — Re: [PATCHv4 08/10] mm/kasan: Switch to using __pa_symbol and lm_alias

On Tue, Nov 29, 2016 at 10:55:27AM -0800, Laura Abbott wrote:
> @@ -94,7 +94,7 @@ static void __init zero_pud_populate(pgd_t *pgd, unsigned long addr,
>  
>  			pud_populate(&init_mm, pud, kasan_zero_pmd);

We also need to lm_alias()-ify kasan_zero_pmd here, or we'll get a
stream of warnings at boot (example below).

I should have spotted that. :/

With that fixed up, I'm able to boot Juno with both KASAN_INLINE and
DEBUG_VIRTUAL, without issued. With that, my previous Reviewed-by and Tested-by
stand.

Thanks,
Mark.

---->8----

[    0.000000] virt_to_phys used for non-linear address :ffff20000a367000
[    0.000000] ------------[ cut here ]------------
[    0.000000] WARNING: CPU: 0 PID: 0 at arch/arm64/mm/physaddr.c:13 __virt_to_phys+0x48/0x68
[    0.000000] Modules linked in:
[    0.000000] 
[    0.000000] CPU: 0 PID: 0 Comm: swapper Not tainted 4.9.0-rc6-00012-gdcc0162-dirty #13
[    0.000000] Hardware name: ARM Juno development board (r1) (DT)
[    0.000000] task: ffff200009ec2200 task.stack: ffff200009eb0000
[    0.000000] PC is at __virt_to_phys+0x48/0x68
[    0.000000] LR is at __virt_to_phys+0x48/0x68
[    0.000000] pc : [<ffff2000080af310>] lr : [<ffff2000080af310>] pstate: 600000c5
[    0.000000] sp : ffff200009eb3c80
[    0.000000] x29: ffff200009eb3c80 x28: ffff20000abdd000 
[    0.000000] x27: ffff200009ce1000 x26: ffff047fffffffff 
[    0.000000] x25: ffff200009ce1000 x24: ffff20000a366100 
[    0.000000] x23: ffff048000000000 x22: ffff20000a366000 
[    0.000000] x21: ffff040080000000 x20: ffff040040000000 
[    0.000000] x19: ffff20000a367000 x18: 000000000000005c 
[    0.000000] x17: 00000009ffec20e0 x16: 00000000fefff4b0 
[    0.000000] x15: ffffffffffffffff x14: 302b646d705f6f72 
[    0.000000] x13: 657a5f6e6173616b x12: 2820303030373633 
[    0.000000] x11: ffff20000a376ca0 x10: 0000000000000010 
[    0.000000] x9 : 646461207261656e x8 : 696c2d6e6f6e2072 
[    0.000000] x7 : 6f66206465737520 x6 : ffff20000a3741e5 
[    0.000000] x5 : 1fffe4000146ee0e x4 : 1fffe400013de704 
[    0.000000] x3 : 1fffe400013d6003 x2 : 1fffe400013d6003 
[    0.000000] x1 : 0000000000000000 x0 : 0000000000000056 
[    0.000000] 
[    0.000000] ---[ end trace 0000000000000000 ]---
[    0.000000] Call trace:
[    0.000000] Exception stack(0xffff200009eb3a50 to 0xffff200009eb3b80)
[    0.000000] 3a40:                                   ffff20000a367000 0001000000000000
[    0.000000] 3a60: ffff200009eb3c80 ffff2000080af310 00000000600000c5 000000000000003d
[    0.000000] 3a80: ffff200009ce1000 ffff2000081c4720 0000000041b58ab3 ffff200009c6cd98
[    0.000000] 3aa0: ffff2000080818a0 ffff20000a366000 ffff048000000000 ffff20000a366100
[    0.000000] 3ac0: ffff200009ce1000 ffff047fffffffff ffff200009ce1000 ffff20000abdd000
[    0.000000] 3ae0: ffff0400013e3ccf ffff20000a3766c0 0000000000000000 0000000000000000
[    0.000000] 3b00: ffff200009eb3c80 ffff200009eb3c80 ffff200009eb3c40 00000000ffffffc8
[    0.000000] 3b20: ffff200009eb3b50 ffff2000082cbd3c ffff200009eb3c80 ffff200009eb3c80
[    0.000000] 3b40: ffff200009eb3c40 00000000ffffffc8 0000000000000056 0000000000000000
[    0.000000] 3b60: 1fffe400013d6003 1fffe400013d6003 1fffe400013de704 1fffe4000146ee0e
[    0.000000] [<ffff2000080af310>] __virt_to_phys+0x48/0x68
[    0.000000] [<ffff200009d734e8>] zero_pud_populate+0x88/0x138
[    0.000000] [<ffff200009d736f8>] kasan_populate_zero_shadow+0x160/0x18c
[    0.000000] [<ffff200009d5a048>] kasan_init+0x1f8/0x408
[    0.000000] [<ffff200009d54000>] setup_arch+0x314/0x948
[    0.000000] [<ffff200009d50c64>] start_kernel+0xb4/0x54c
[    0.000000] [<ffff200009d501e0>] __primary_switched+0x64/0x74

[mark@leverpostej:~/src/linux]% uselinaro 15.08 aarch64-linux-gnu-readelf -s vmlinux | grep ffff20000a367000
108184: ffff20000a367000  4096 OBJECT  GLOBAL DEFAULT   25 kasan_zero_pmd

[mark@leverpostej:~/src/linux]% uselinaro 15.08 aarch64-linux-gnu-addr2line -ife vmlinux ffff200009d734e8              
set_pud
/home/mark/src/linux/./arch/arm64/include/asm/pgtable.h:435
__pud_populate
/home/mark/src/linux/./arch/arm64/include/asm/pgalloc.h:47
pud_populate
/home/mark/src/linux/./arch/arm64/include/asm/pgalloc.h:52
zero_pud_populate
/home/mark/src/linux/mm/kasan/kasan_init.c:95

[toc] | [prev] | [next] | [standalone]

#1532680 — [PATCHv4 06/10] xen: Switch to using __pa_symbol

__pa_symbol is the correct macro to use on kernel
symbols. Switch to this from __pa.

Signed-off-by: Laura Abbott <labbott@redhat.com>
---
Found during a sweep of the kernel. Untested.
---
 drivers/xen/xenbus/xenbus_dev_backend.c | 2 +-
 drivers/xen/xenfs/xenstored.c           | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/xen/xenbus/xenbus_dev_backend.c b/drivers/xen/xenbus/xenbus_dev_backend.c
index 4a41ac9..31ca2bf 100644
--- a/drivers/xen/xenbus/xenbus_dev_backend.c
+++ b/drivers/xen/xenbus/xenbus_dev_backend.c
@@ -99,7 +99,7 @@ static int xenbus_backend_mmap(struct file *file, struct vm_area_struct *vma)
 		return -EINVAL;
 
 	if (remap_pfn_range(vma, vma->vm_start,
-			    virt_to_pfn(xen_store_interface),
+			    PHYS_PFN(__pa_symbol(xen_store_interface)),
 			    size, vma->vm_page_prot))
 		return -EAGAIN;
 
diff --git a/drivers/xen/xenfs/xenstored.c b/drivers/xen/xenfs/xenstored.c
index fef20db..21009ea 100644
--- a/drivers/xen/xenfs/xenstored.c
+++ b/drivers/xen/xenfs/xenstored.c
@@ -38,7 +38,7 @@ static int xsd_kva_mmap(struct file *file, struct vm_area_struct *vma)
 		return -EINVAL;
 
 	if (remap_pfn_range(vma, vma->vm_start,
-			    virt_to_pfn(xen_store_interface),
+			    PHYS_PFN(__pa_symbol(xen_store_interface)),
 			    size, vma->vm_page_prot))
 		return -EAGAIN;
 
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1532821 — Re: [PATCHv4 06/10] xen: Switch to using __pa_symbol

On 11/29/2016 01:55 PM, Laura Abbott wrote:
> __pa_symbol is the correct macro to use on kernel
> symbols. Switch to this from __pa.
>
> Signed-off-by: Laura Abbott <labbott@redhat.com>
> ---
> Found during a sweep of the kernel. Untested.
> ---
>  drivers/xen/xenbus/xenbus_dev_backend.c | 2 +-
>  drivers/xen/xenfs/xenstored.c           | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/xen/xenbus/xenbus_dev_backend.c b/drivers/xen/xenbus/xenbus_dev_backend.c
> index 4a41ac9..31ca2bf 100644
> --- a/drivers/xen/xenbus/xenbus_dev_backend.c
> +++ b/drivers/xen/xenbus/xenbus_dev_backend.c
> @@ -99,7 +99,7 @@ static int xenbus_backend_mmap(struct file *file, struct vm_area_struct *vma)
>  		return -EINVAL;
>  
>  	if (remap_pfn_range(vma, vma->vm_start,
> -			    virt_to_pfn(xen_store_interface),
> +			    PHYS_PFN(__pa_symbol(xen_store_interface)),
>  			    size, vma->vm_page_prot))
>  		return -EAGAIN;
>  
> diff --git a/drivers/xen/xenfs/xenstored.c b/drivers/xen/xenfs/xenstored.c
> index fef20db..21009ea 100644
> --- a/drivers/xen/xenfs/xenstored.c
> +++ b/drivers/xen/xenfs/xenstored.c
> @@ -38,7 +38,7 @@ static int xsd_kva_mmap(struct file *file, struct vm_area_struct *vma)
>  		return -EINVAL;
>  
>  	if (remap_pfn_range(vma, vma->vm_start,
> -			    virt_to_pfn(xen_store_interface),
> +			    PHYS_PFN(__pa_symbol(xen_store_interface)),
>  			    size, vma->vm_page_prot))
>  		return -EAGAIN;
>  


I suspect this won't work --- xen_store_interface doesn't point to a
kernel symbol.

-boris

[toc] | [prev] | [next] | [standalone]

#1532829 — Re: [PATCHv4 06/10] xen: Switch to using __pa_symbol

On 11/29/2016 02:26 PM, Boris Ostrovsky wrote:
> On 11/29/2016 01:55 PM, Laura Abbott wrote:
>> __pa_symbol is the correct macro to use on kernel
>> symbols. Switch to this from __pa.
>>
>> Signed-off-by: Laura Abbott <labbott@redhat.com>
>> ---
>> Found during a sweep of the kernel. Untested.
>> ---
>>  drivers/xen/xenbus/xenbus_dev_backend.c | 2 +-
>>  drivers/xen/xenfs/xenstored.c           | 2 +-
>>  2 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/xen/xenbus/xenbus_dev_backend.c b/drivers/xen/xenbus/xenbus_dev_backend.c
>> index 4a41ac9..31ca2bf 100644
>> --- a/drivers/xen/xenbus/xenbus_dev_backend.c
>> +++ b/drivers/xen/xenbus/xenbus_dev_backend.c
>> @@ -99,7 +99,7 @@ static int xenbus_backend_mmap(struct file *file, struct vm_area_struct *vma)
>>  		return -EINVAL;
>>  
>>  	if (remap_pfn_range(vma, vma->vm_start,
>> -			    virt_to_pfn(xen_store_interface),
>> +			    PHYS_PFN(__pa_symbol(xen_store_interface)),
>>  			    size, vma->vm_page_prot))
>>  		return -EAGAIN;
>>  
>> diff --git a/drivers/xen/xenfs/xenstored.c b/drivers/xen/xenfs/xenstored.c
>> index fef20db..21009ea 100644
>> --- a/drivers/xen/xenfs/xenstored.c
>> +++ b/drivers/xen/xenfs/xenstored.c
>> @@ -38,7 +38,7 @@ static int xsd_kva_mmap(struct file *file, struct vm_area_struct *vma)
>>  		return -EINVAL;
>>  
>>  	if (remap_pfn_range(vma, vma->vm_start,
>> -			    virt_to_pfn(xen_store_interface),
>> +			    PHYS_PFN(__pa_symbol(xen_store_interface)),
>>  			    size, vma->vm_page_prot))
>>  		return -EAGAIN;
>>  
> 
> 
> I suspect this won't work --- xen_store_interface doesn't point to a
> kernel symbol.
> 
> -boris
> 

I reviewed this again and yes you are right. I missed that this
was a pointer and not just a symbol so I think this patch can
just be dropped.

Thanks,
Laura

[toc] | [prev] | [next] | [standalone]

#1537248 — [PATCH 0/3] ARM: Add support for CONFIG_DEBUG_VIRTUAL

Hi all,

This patch series builds on top of Laura's [PATCHv4 00/10] CONFIG_DEBUG_VIRTUAL for arm64
to add support for CONFIG_DEBUG_VIRTUAL for ARM.

This was tested on a Brahma B15 platform (ARMv7 + HIGHMEM + LPAE).

There are a number of possible follow up/cleanup patches:

- all SMP implements that pass down the address of secondary_startup to the SMP bringup
  operations should use __pa_symbol() instead since they reference in-kernel symbols

Flames, critiques, rotten tomatoes welcome!

Florian Fainelli (3):
  ARM: Define KERNEL_START and KERNEL_END
  ARM: Utilize __pa_symbol in lieu of __pa
  ARM: Add support for CONFIG_DEBUG_VIRTUAL

 arch/arm/Kconfig              |  1 +
 arch/arm/include/asm/memory.h | 23 +++++++++++++++++--
 arch/arm/mm/Makefile          |  1 +
 arch/arm/mm/init.c            |  7 ++----
 arch/arm/mm/mmu.c             | 10 +++------
 arch/arm/mm/physaddr.c        | 51 +++++++++++++++++++++++++++++++++++++++++++
 6 files changed, 79 insertions(+), 14 deletions(-)
 create mode 100644 arch/arm/mm/physaddr.c

-- 
2.9.3

[toc] | [prev] | [next] | [standalone]

#1537250 — [PATCH 3/3] ARM: Add support for CONFIG_DEBUG_VIRTUAL

x86 has an option: CONFIG_DEBUG_VIRTUAL to do additional checks on
virt_to_phys calls. The goal is to catch users who are calling
virt_to_phys on non-linear addresses immediately. This includes caller
using __virt_to_phys() on image addresses instead of __pa_symbol(). This
is a generally useful debug feature to spot bad code (particulary in
drivers).

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 arch/arm/Kconfig              |  1 +
 arch/arm/include/asm/memory.h | 16 ++++++++++++--
 arch/arm/mm/Makefile          |  1 +
 arch/arm/mm/physaddr.c        | 51 +++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 67 insertions(+), 2 deletions(-)
 create mode 100644 arch/arm/mm/physaddr.c

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index b5d529fdffab..5e66173c5787 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -2,6 +2,7 @@ config ARM
 	bool
 	default y
 	select ARCH_CLOCKSOURCE_DATA
+	select ARCH_HAS_DEBUG_VIRTUAL
 	select ARCH_HAS_DEVMEM_IS_ALLOWED
 	select ARCH_HAS_ELF_RANDOMIZE
 	select ARCH_HAS_TICK_BROADCAST if GENERIC_CLOCKEVENTS_BROADCAST
diff --git a/arch/arm/include/asm/memory.h b/arch/arm/include/asm/memory.h
index bee7511c5098..46f192218be7 100644
--- a/arch/arm/include/asm/memory.h
+++ b/arch/arm/include/asm/memory.h
@@ -213,7 +213,7 @@ extern const void *__pv_table_begin, *__pv_table_end;
 	: "r" (x), "I" (__PV_BITS_31_24)		\
 	: "cc")
 
-static inline phys_addr_t __virt_to_phys(unsigned long x)
+static inline phys_addr_t __virt_to_phys_nodebug(unsigned long x)
 {
 	phys_addr_t t;
 
@@ -245,7 +245,7 @@ static inline unsigned long __phys_to_virt(phys_addr_t x)
 #define PHYS_OFFSET	PLAT_PHYS_OFFSET
 #define PHYS_PFN_OFFSET	((unsigned long)(PHYS_OFFSET >> PAGE_SHIFT))
 
-static inline phys_addr_t __virt_to_phys(unsigned long x)
+static inline phys_addr_t __virt_to_phys_nodebug(unsigned long x)
 {
 	return (phys_addr_t)x - PAGE_OFFSET + PHYS_OFFSET;
 }
@@ -261,6 +261,16 @@ static inline unsigned long __phys_to_virt(phys_addr_t x)
 	((((unsigned long)(kaddr) - PAGE_OFFSET) >> PAGE_SHIFT) + \
 	 PHYS_PFN_OFFSET)
 
+#define __pa_symbol_nodebug(x)	((x) - (unsigned long)KERNEL_START)
+
+#ifdef CONFIG_DEBUG_VIRTUAL
+extern phys_addr_t __virt_to_phys(unsigned long x);
+extern phys_addr_t __phys_addr_symbol(unsigned long x);
+#else
+#define __virt_to_phys(x)	__virt_to_phys_nodebug(x)
+#define __phys_addr_symbol(x)	__pa_symbol_nodebug(x)
+#endif
+
 /*
  * These are *only* valid on the kernel direct mapped RAM memory.
  * Note: Drivers should NOT use these.  They are the wrong
@@ -283,9 +293,11 @@ static inline void *phys_to_virt(phys_addr_t x)
  * Drivers should NOT use these either.
  */
 #define __pa(x)			__virt_to_phys((unsigned long)(x))
+#define __pa_symbol(x)		__phys_addr_symbol(RELOC_HIDE((unsigned long)(x), 0))
 #define __va(x)			((void *)__phys_to_virt((phys_addr_t)(x)))
 #define pfn_to_kaddr(pfn)	__va((phys_addr_t)(pfn) << PAGE_SHIFT)
 
+
 extern long long arch_phys_to_idmap_offset;
 
 /*
diff --git a/arch/arm/mm/Makefile b/arch/arm/mm/Makefile
index e8698241ece9..b3dea80715b4 100644
--- a/arch/arm/mm/Makefile
+++ b/arch/arm/mm/Makefile
@@ -14,6 +14,7 @@ endif
 
 obj-$(CONFIG_ARM_PTDUMP)	+= dump.o
 obj-$(CONFIG_MODULES)		+= proc-syms.o
+obj-$(CONFIG_DEBUG_VIRTUAL)	+= physaddr.o
 
 obj-$(CONFIG_ALIGNMENT_TRAP)	+= alignment.o
 obj-$(CONFIG_HIGHMEM)		+= highmem.o
diff --git a/arch/arm/mm/physaddr.c b/arch/arm/mm/physaddr.c
new file mode 100644
index 000000000000..00f6dcffab8b
--- /dev/null
+++ b/arch/arm/mm/physaddr.c
@@ -0,0 +1,51 @@
+#include <linux/bug.h>
+#include <linux/export.h>
+#include <linux/types.h>
+#include <linux/mmdebug.h>
+#include <linux/mm.h>
+
+#include <asm/sections.h>
+#include <asm/memory.h>
+#include <asm/fixmap.h>
+
+#include "mm.h"
+
+static inline bool __virt_addr_valid(unsigned long x)
+{
+	if (x < PAGE_OFFSET)
+		return false;
+	if (arm_lowmem_limit && is_vmalloc_or_module_addr((void *)x))
+		return false;
+	if (x >= FIXADDR_START && x < FIXADDR_END)
+		return false;
+	return true;
+}
+
+phys_addr_t __virt_to_phys(unsigned long x)
+{
+	WARN(!__virt_addr_valid(x),
+	     "virt_to_phys used for non-linear address :%pK\n", (void *)x);
+
+	return __virt_to_phys_nodebug(x);
+}
+EXPORT_SYMBOL(__virt_to_phys);
+
+static inline bool __phys_addr_valid(unsigned long x)
+{
+	/* This is bounds checking against the kernel image only.
+	 * __pa_symbol should only be used on kernel symbol addresses.
+	 */
+	if (x < (unsigned long)KERNEL_START ||
+	    x > (unsigned long)KERNEL_END)
+		return false;
+
+	return true;
+}
+
+phys_addr_t __phys_addr_symbol(unsigned long x)
+{
+	VIRTUAL_BUG_ON(!__phys_addr_valid(x));
+
+	return __pa_symbol_nodebug(x);
+}
+EXPORT_SYMBOL(__phys_addr_symbol);
-- 
2.9.3

[toc] | [prev] | [next] | [standalone]

#1537266 — Re: [PATCH 3/3] ARM: Add support for CONFIG_DEBUG_VIRTUAL

On 12/06/2016 11:53 AM, Florian Fainelli wrote:
> x86 has an option: CONFIG_DEBUG_VIRTUAL to do additional checks on
> virt_to_phys calls. The goal is to catch users who are calling
> virt_to_phys on non-linear addresses immediately. This includes caller
> using __virt_to_phys() on image addresses instead of __pa_symbol(). This
> is a generally useful debug feature to spot bad code (particulary in
> drivers).
> 
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---

> @@ -261,6 +261,16 @@ static inline unsigned long __phys_to_virt(phys_addr_t x)
>  	((((unsigned long)(kaddr) - PAGE_OFFSET) >> PAGE_SHIFT) + \
>  	 PHYS_PFN_OFFSET)
>  
> +#define __pa_symbol_nodebug(x)	((x) - (unsigned long)KERNEL_START)

I don't think I got this one quite right, but I also assume that won't
be the only problem with this patch series.
-- 
Florian

[toc] | [prev] | [next] | [standalone]

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web