linux-kernel - Re: [PATCH] ARM: fix alignement of __bug

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]

Date:	Thu, 10 Sep 2015 22:53:43 +0200
From:	Robert Jarzmik <robert.jarzmik@...e.fr>
To:	Russell King - ARM Linux <linux@....linux.org.uk>
Cc:	Dave Martin <Dave.Martin@....com>, linux-kernel@...r.kernel.org,
	linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH] ARM: fix alignement of __bug_table section entries

Russell King - ARM Linux <linux@....linux.org.uk> writes:

> I've been wondering whether we can teach GCC that set_domain modifies
> the value that get_domain returns, rather than throwing a volatile
> onto the asm in get_domain.  The issue with a volatile there is that
> even if the result is unused, but the code is reachable, gcc still has
> to output the code to read the register.
>
> We might be able to get away with a memory clobber on the set_domain,
> and fake a memory read in get_domain, eg, by passing
> 	"m" (current_thread_info()->cpu_domain))
> to the get_domain asm.
Ok, let's say we do it that way.

I have some concerns about it:
  (a) I see an inbalance, as set_domain() doesn't actually modify
      current_thread_info()->cpu_domain. I don't see how it will protect use
      from this scenario :
        - get_domain()
        - set_domain()
        - set_domain()

  (b) domain.h is included from thread_info.h, not the other way around
      => current_thread_info() is not accessible from domain.h
      => that would require a bit of moving things around, as thread_info
         structure description should be available for example.
      This is currently my biggest problem with this approach.

  (c) I was also wondering if a case like this could happen :
     - a function foo() does a get_domain()
       => an exception/irq whatever happens and modifies the DACR
     - foo() continues a makes a modify_domain()
       => and here the modify_domain() uses the old DACR value
      Or said differently, I wonder if there is a case of 2 get_domain() calls
      in a row with a DACR modification in between. I

What about something such as [1], without a memory clobber, but a "fake" memory
variable link ?

Cheers.

--
Robert

[1] get_domain() / set_domain() link
diff --git a/arch/arm/include/asm/domain.h b/arch/arm/include/asm/domain.h
index e878129f2fee..fc1d9c43aa08 100644
--- a/arch/arm/include/asm/domain.h
+++ b/arch/arm/include/asm/domain.h
@@ -83,13 +83,17 @@
 
 #ifndef __ASSEMBLY__
 
+static int domain_barrier;
+/*
+ * how to get the current stack pointer in C
+ */
 static inline unsigned int get_domain(void)
 {
        unsigned int domain;
 
        asm(
        "mrc    p15, 0, %0, c3, c0      @ get domain"
-        : "=r" (domain));
+        : "=r" (domain), "=m" (domain_barrier));
 
        return domain;
 }
@@ -97,8 +101,8 @@ static inline unsigned int get_domain(void)
 static inline void set_domain(unsigned val)
 {
        asm volatile(
-       "mcr    p15, 0, %0, c3, c0      @ set domain"
-         : : "r" (val));
+       "mcr    p15, 0, %1, c3, c0      @ set domain"
+       : "=m" (domain_barrier) : "r" (val));
        isb();
 }
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/