Re: [Qemu-devel] x86 segment limits enforcement with TCG

[Stephen Checkoway]

FWIW, I figured out an approach to this. Essentially, I'm reusing the function 
which computes linear addresses to enforce not only segment limits (in 
everything but long mode), but also read/write access (in protected mode).

Unfortunately, that meant every call to the linear address computation has to 
be augmented with an access size and whether it's a store or not. The patch is 
pretty large so I won't include it here, but you can view it at 
<https://github.com/stevecheckoway/qemu/commit/ac58652efacedc53f3831301ea0326ac8f882b18>.

If this is something that qemu would like, then I think two additional things 
are definitely required:
1. Tests. make check passes and the firmware I have which necessitated the 
checks appears to work, but that change touches the almost every 
guest-memory-accessing x86 instruction.
2. This is going to slow down the common case—where segments have a 0 base 
address and a limit of 0xFFFFFFFF—and there's no real need to do that. It seems 
like something akin to addseg could be used to decide when to insert the 
checks. I don't really understand how that works and in my case, segments with 
nonzero bases and non-0xFFFFFFFF limits are the norm so I didn't investigate 
that. Something similar could probably be done to omit the writable segment 
checks.

Finally, there are some limitations. The amount of memory touched by xsave (and 
the related instructions) depends on edx:eax. I didn't bother checking that at 
all. Similarly, there are some MPX instructions that don't do any access checks 
when they really should. And finally, there's one place that checks for an 
access size of 8 bytes when, in some cases, it should be 16.

I'm happy to work to upstream this, if the approach is broadly acceptable and 
the functionality is desired.

If not, thank you Peter for answering my questions which enabled me to solve my 
problem.

Cheers,

Steve

> On Feb 24, 2019, at 15:21, Stephen Checkoway <address@hidden> wrote:
> 
> 
> 
>> On Feb 24, 2019, at 14:46, Peter Maydell <address@hidden> wrote:
>> 
>> On Sun, 24 Feb 2019 at 19:37, Stephen Checkoway
>> <address@hidden> wrote:
>>> I think that something about adding the tcg_gen_brcond_tl is causing values 
>>> to become dead and then qemu aborts.
>> 
>> Yep -- all "TCG temporaries" are dead at the end
>> of a basic block, and brcond ends a basic block.
>> Only globals and "local temporaries" stay live
>> across brcond. This is documented in tcg/README,
>> though it doesn't spell it out very explicitly.
> 
> Ah yes. I see that now. I missed it on my first read through.
> 
>> This makes brcond pretty painful to use and
>> almost impossible to introduce into the middle
>> of some existing sequence of generated code.
>> I haven't looked at what the best way to do what
>> you're trying to do here is, though.
> 
> Are there other examples of straight-line code being converted to a 
> conditional I might be able to use as an example? I thought INTO would be a 
> good example, but it merely calls a helper. Maybe I should do that? I assume 
> it'll be slow, but speed isn't really my primary concern.
> 
>> By the way, don't do this:
>> +    dc->A1 = tcg_temp_new();
>> 
>> The current use of a small number of tcg temps
>> in the i386 translate.c code is an antipattern
>> that is a relic from a very old version of the
>> code. It's much better to simply create new
>> temporaries in the code at the point where you
>> need them and then free them once you're done.
> 
> Great, thanks. I saw both the A0/T0/T1 and the creation of new temporaries 
> and I wasn't sure which pattern I should follow.
> 
> -- 
> Stephen Checkoway
> 
> 
> 
> 
> 

-- 
Stephen Checkoway