Re: [PATCH] intel_txt: add s3 userspace memory integrity verification

[Pavel Machek <pavel@ucw.cz>]

Hi!

Please wrap mails at column 72 (or so).

> > AFAICT, it verifies userspace _and_ kernel memory, that's why it does
> > magic stack switching. Why not verify everything in tboot?
> Because tboot only can access <4G mem without paging. And the memory is sparse. We can't/needn't set unlimited sparse mem ranges to the MAC array with limited elements in the shared page, in order to pass the parameters.
> On the other hand, it is reasonable for tboot to verify kernel, and kernel to verify userspace memory.
>

Are  you sure x86-64 kernel & modules is always below 4GB? I don't
think so.

 
> >> +static vmac_t mem_mac;
> >> +static struct crypto_hash *tfm;
> > 
> > Could these be automatic?
> Maybe, but I don't wish other files can access the variables and take tfm as an example, I'd like to allocate memory to it once and then initialize it once in order to avoid impact of memory change to MACing.
>

You use stack, anyway.

> > Why does 4G limit matter on 64-bit?
> tboot can't access >4G, see above.

Too bad, then its broken by design.

> >> +	if (tboot_gen_mem_integrity(tboot->s3_key, &mac))
> >> +		panic("tboot: vmac generation failed\n");
> >> +	else if (mac != mem_mac)
> >> +		panic("tboot: memory integrity was lost on resume\n"); +	else
> >> +		pr_info("memory integrity OK\n");
> > 
> > So I corrupt memory, but also corrupt tboot_enabled() to return 0....
> > 
> > And... does panic kill the machine quickly enough that no 'bad stuff'
> > happens? (Whats bad stuff in this context, anyway?).

I'd really like you to answer that.


> >> @@ -244,7 +245,10 @@ static int acpi_suspend_enter(suspend_st 
> >> break; 
> >> 
> >>  	case ACPI_STATE_S3:
> >> +		tboot_switch_stack();
> >>  		do_suspend_lowlevel();
> >> +		tboot_sx_resume();
> >> +		tboot_restore_stack();
> >>  		break;
> >>  	}
> >> 
> > 
> > Did you audit all code before sx_resume()? If it trusts  data not
> > checksummed by tboot, attacker may be able to hijack code execution
> > and bypass your protection, no?
> Yes, kernel code is audited by tboot before resume.

So no, you did not audit do_suspend_lowlevel to make sure it does not
follow function pointers. Bad.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html