Radim, Paolo, Sorry for the late responses (due to holidays)…
On Oct 11, 2014, at 12:39 PM, Paolo Bonzini <pbonzini@xxxxxxxxxx> wrote:
> Il 10/10/2014 17:54, Radim Krčmář ha scritto:
>>>>
>>>> One exception is the case of conforming code segment. The SDM says: "Use a
>>>> code-segment override prefix (CS) to read a readable... [it is] valid because
>>>> the DPL of the code segment selected by the CS register is the same as the
>>>> CPL." This is misleading since CS.DPL may be lower (numerically) than CPL, and
>>>> CS would still be accessible. The emulator should avoid privilage level checks
>>>> for data reads using CS.
>> Ah, after stripping faulty presumptions, I'm not sure this change is
>> enough ... shouldn't we also skip the check on conforming code segments?
>>
>> Method 2 is always valid because the privilege level of a conforming
>> code segment is effectively the same as the CPL, regardless of its DPL.
>
> Radim is right; we need to skip the check on conforming code segments
> and, once we do that, checking addr.seg is not necessary anymore. That
> is because, for a CS override on a nonconforming code segment, at the
> time we fetch the instruction we know that cpl == desc.dpl. The less
> restrictive data segment check (cpl <= desc.dpl) thus always passes.
Yes. I was wrong, assuming the code-segment checks are just a derivative of the data segment checks.
>
> Let's put together this check and the readability check, too, since
> we are adding another "if (fetch)".
>
> Can you guys think of a way to simplify the following untested patch?
>
> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
> index 03954f7900f5..9f3e33551db9 100644
> --- a/arch/x86/kvm/emulate.c
> +++ b/arch/x86/kvm/emulate.c
> @@ -638,9 +638,6 @@ static int __linearize(struct x86_emulate_ctxt *ctxt,
> if ((((ctxt->mode != X86EMUL_MODE_REAL) && (desc.type & 8))
> || !(desc.type & 2)) && write)
> goto bad;
> - /* unreadable code segment */
> - if (!fetch && (desc.type & 8) && !(desc.type & 2))
> - goto bad;
> lim = desc_limit_scaled(&desc);
> if ((ctxt->mode == X86EMUL_MODE_REAL) && !fetch &&
> (ctxt->d & NoBigReal)) {
> @@ -660,17 +657,40 @@ static int __linearize(struct x86_emulate_ctxt *ctxt,
> goto bad;
> }
> cpl = ctxt->ops->cpl(ctxt);
> - if (!(desc.type & 8)) {
> - /* data segment */
> + if (fetch && (desc.type & 8)) {
> + if (!(desc.type & 4)) {
> + /* nonconforming code segment */
> + if (cpl != desc.dpl)
> + goto bad;
> + break;
> + } else {
> + /* conforming code segment */
> + if (cpl < desc.dpl)
> + goto bad;
> + break;
> + }
> + }
> +
> + if (likely(!(desc.type & 8) || (desc.type & 6) == 2)) {
> + /*
> + * Data segment or readable, nonconforming code
> + * segment. The SDM mentions that access through
> + * a code-segment override prefix is always valid.
> + * This really only matters for conforming code
> + * segments (checked below, and always valid anyway):
> + * for nonconforming ones, cpl == desc.dpl was checked
> + * when fetching the instruction, meaning the following
> + * test will always pass too.
> + */
> if (cpl > desc.dpl)
> goto bad;
> - } else if ((desc.type & 8) && !(desc.type & 4)) {
> - /* nonconforming code segment */
> - if (cpl != desc.dpl)
> - goto bad;
> - } else if ((desc.type & 8) && (desc.type & 4)) {
> - /* conforming code segment */
> - if (cpl < desc.dpl)
> + } else {
> + /*
> + * These are the (rare) cases that do not behave
> + * like data segments: nonreadable code segments (bad)
> + * and readable, conforming code segments (good).
> + */
> + if (!(desc.type & 2))
> goto bad;
> }
> break;
Looks good. I’ll give it a try but it is hard to give a definitive answer, since the emulator is still bug-ridden.
Please note I submitted another patch at this area ("Wrong error code on limit violation during emulation”), so both should be merged.
Thanks,
Nadav
Attachment:
signature.asc
Description: Message signed with OpenPGP using GPGMail
