Re: [PATCH v4] hwmon: (dell-smm) Improve assembly code

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Am 22.02.22 um 20:46 schrieb David Laight:

From: Pali Rohár
Sent: 22 February 2022 17:52

On Tuesday 22 February 2022 08:54:32 Guenter Roeck wrote:
On Sun, Feb 20, 2022 at 08:08:51PM +0100, Armin Wolf wrote:
The new assembly code works on both 32 bit and 64 bit
cpus and allows for more compiler optimisations.
Since clang runs out of registers on 32 bit x86 when
using CC_OUT, we need to execute "setc" ourself.
Also modify the debug message so we can still see
the result (eax) when the carry flag was set.

Tested with 32 bit and 64 bit kernels on a Dell Inspiron 3505.

Signed-off-by: Armin Wolf <W_Armin@xxxxxx>
It would be great if I can get some Tested-by/Acked-by/Reviewed-by
tags for this patch.
Well, I know about this driver asm code for a long time and it since
beginning it was suspicious for me, why there is such huge code with
stack and registers manipulation and why it cannot be implemented easily
via just two "out" instructions. This patch is exactly doing it.
But question reminds why it was written in this simple way since
beginning.

If this change is correct then I have no problem with it.

But I would rather see review of this asm change by skilled x86 asm
developer. We are dealing here with CPU 0, SMM x86 mode, I/O ports and
stack manipulation in inline gcc asm which sounds like a trap. And I'm
not feeling skilled for reviewing this change.

May I ask somebody to review this change? Is there some linux x86 ML?
On the face of it the new asm code is basically the same as the old
asm code - and both are probably equivalent to the reverse engineered
windows code.

The real problem isn't the new code, it is more 'WTF is actually going on'.
Somehow the pair of outb are generating a synchronous (or synchronous
enough) trap that the trap handler (in the smm code in the bios)
can change the registers before the code following the outb saves
them back to memory.

SMM mode is a semi-hidden cpu mode usually entered by an interrupt.
It is horrid and nasty because it has access to the cpu memory and
registers, but is hidden and unknown (I think in the bios).

Normal bios calls can (usually) be executed using a cpu emulator
(especially useful for doing real-mode call from 64bit mode) so
the kernel can limit what they can do.

But SMM mode is nearly as bad as the ME processor on newer systems.
If you worry about security you really want it disabled - but you can't.

As far as this patch goes I think I'd add a 'stc' (set carry)
instruction before the first 'outb'.
Then if the 'something magic happens here' doesn't happen (because
you aren't on the right kind of motherboard) the action fails.

	David

We already check for such scenarios by checking if eax changed.
I would not set the carry flag before doing a SMM call since im
not sure if the firmware code always clears the carry flag when
the call was successful.
If for example the firmware code only sets the carry flag on
error and otherwise ignores the carry flag, we might create some
false negatives when a successful SMM call leaves the carry flag set.

Thanks,
Guenter

---
Changes in v4:
- reword commit message

Changes in v3:
- make carry an unsigned char
- use "+a", ... for output registers
- drop "cc" from clobbered list

Changes in v2:
- fix clang running out of registers on 32 bit x86
- modify debug message
---
  drivers/hwmon/dell-smm-hwmon.c | 78 ++++++++--------------------------
  1 file changed, 18 insertions(+), 60 deletions(-)

--
2.30.2

diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
index c5939e68586d..38d23a8e83f2 100644
--- a/drivers/hwmon/dell-smm-hwmon.c
+++ b/drivers/hwmon/dell-smm-hwmon.c
@@ -119,7 +119,7 @@ struct smm_regs {
  	unsigned int edx;
  	unsigned int esi;
  	unsigned int edi;
-} __packed;
+};

  static const char * const temp_labels[] = {
  	"CPU",
@@ -164,74 +164,32 @@ static int i8k_smm_func(void *par)
  	struct smm_regs *regs = par;
  	int eax = regs->eax;
  	int ebx = regs->ebx;
+	unsigned char carry;
  	long long duration;
-	int rc;

  	/* SMM requires CPU 0 */
  	if (smp_processor_id() != 0)
  		return -EBUSY;

-#if defined(CONFIG_X86_64)
-	asm volatile("pushq %%rax\n\t"
-		"movl 0(%%rax),%%edx\n\t"
-		"pushq %%rdx\n\t"
-		"movl 4(%%rax),%%ebx\n\t"
-		"movl 8(%%rax),%%ecx\n\t"
-		"movl 12(%%rax),%%edx\n\t"
-		"movl 16(%%rax),%%esi\n\t"
-		"movl 20(%%rax),%%edi\n\t"
-		"popq %%rax\n\t"
-		"out %%al,$0xb2\n\t"
-		"out %%al,$0x84\n\t"
-		"xchgq %%rax,(%%rsp)\n\t"
-		"movl %%ebx,4(%%rax)\n\t"
-		"movl %%ecx,8(%%rax)\n\t"
-		"movl %%edx,12(%%rax)\n\t"
-		"movl %%esi,16(%%rax)\n\t"
-		"movl %%edi,20(%%rax)\n\t"
-		"popq %%rdx\n\t"
-		"movl %%edx,0(%%rax)\n\t"
-		"pushfq\n\t"
-		"popq %%rax\n\t"
-		"andl $1,%%eax\n"
-		: "=a"(rc)
-		:    "a"(regs)
-		:    "%ebx", "%ecx", "%edx", "%esi", "%edi", "memory");
-#else
-	asm volatile("pushl %%eax\n\t"
-	    "movl 0(%%eax),%%edx\n\t"
-	    "push %%edx\n\t"
-	    "movl 4(%%eax),%%ebx\n\t"
-	    "movl 8(%%eax),%%ecx\n\t"
-	    "movl 12(%%eax),%%edx\n\t"
-	    "movl 16(%%eax),%%esi\n\t"
-	    "movl 20(%%eax),%%edi\n\t"
-	    "popl %%eax\n\t"
-	    "out %%al,$0xb2\n\t"
-	    "out %%al,$0x84\n\t"
-	    "xchgl %%eax,(%%esp)\n\t"
-	    "movl %%ebx,4(%%eax)\n\t"
-	    "movl %%ecx,8(%%eax)\n\t"
-	    "movl %%edx,12(%%eax)\n\t"
-	    "movl %%esi,16(%%eax)\n\t"
-	    "movl %%edi,20(%%eax)\n\t"
-	    "popl %%edx\n\t"
-	    "movl %%edx,0(%%eax)\n\t"
-	    "lahf\n\t"
-	    "shrl $8,%%eax\n\t"
-	    "andl $1,%%eax\n"
-	    : "=a"(rc)
-	    :    "a"(regs)
-	    :    "%ebx", "%ecx", "%edx", "%esi", "%edi", "memory");
-#endif
-	if (rc != 0 || (regs->eax & 0xffff) == 0xffff || regs->eax == eax)
-		rc = -EINVAL;
+	asm volatile("out %%al,$0xb2\n\t"
+		     "out %%al,$0x84\n\t"
+		     "setc %0\n"
+		     : "=mr" (carry),
+		       "+a" (regs->eax),
+		       "+b" (regs->ebx),
+		       "+c" (regs->ecx),
+		       "+d" (regs->edx),
+		       "+S" (regs->esi),
+		       "+D" (regs->edi));

  	duration = ktime_us_delta(ktime_get(), calltime);
-	pr_debug("smm(0x%.4x 0x%.4x) = 0x%.4x  (took %7lld usecs)\n", eax, ebx,
-		 (rc ? 0xffff : regs->eax & 0xffff), duration);
+	pr_debug("smm(0x%.4x 0x%.4x) = 0x%.4x carry: %d (took %7lld usecs)\n",
+		 eax, ebx, regs->eax & 0xffff, carry, duration);

-	return rc;
+	if (carry || (regs->eax & 0xffff) == 0xffff || regs->eax == eax)
+		return -EINVAL;
+
+	return 0;
  }

  /*
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)





[Index of Archives]     [LM Sensors]     [Linux Sound]     [ALSA Users]     [ALSA Devel]     [Linux Audio Users]     [Linux Media]     [Kernel]     [Gimp]     [Yosemite News]     [Linux Media]

  Powered by Linux