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

[David Laight <David.Laight@xxxxxxxxxx>]

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

> 
> > 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)