| Message ID | 5438FAD6.3010805@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Radim, Paolo, Sorry for the late responses (due to holidays)… On Oct 11, 2014, at 12:39 PM, Paolo Bonzini <pbonzini@redhat.com> 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
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Il 12/10/2014 08:57, Nadav Amit ha scritto: > Looks good. I’ll give it a try but it is hard to give a definitive > answer, since the emulator is still bug-ridden. Yes, we need to write unit tests for this, especially the conforming case. A bit of a pain to get kvm-unit-tests in ring 3 (access.flat does it), but I'll give it a shot. Paolo -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQEcBAEBAgAGBQJUOnBCAAoJEBRUblpOawnXLHEH/3zlIsVFow9IWrZsxaZopWy5 DKncQriHnhsyc6W2U9oFvNgB/7+dTWscdR58jnKLr/Qt64DGH01pq4MvisdhV+31 53pv+CycUgs85EoZkA7MzArbT1Tb/gd/KB8QoqdKIC4+bNEd6JMydcsq5d6nMgOd yWLYjcYzWzzmJSNCC7UYOtN4SB4brC5RyITLq+CgT4ufSPHtBYxGd8fSHpzvJzTU T1hqelYcLRFGzoPR4ux4SP8EgXle+sslFV4KAyXkucLIafmeekcmR/AO8hy84TXj Kcpt6Y3hsY8pWXM3YB4LF/7+NuaPu/Ud+2VkbfuFhq5gUtVLwjtWtA32IlYdFEc= =BhUU -----END PGP SIGNATURE----- -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10/12/14 3:12 PM, Paolo Bonzini wrote: > Il 12/10/2014 08:57, Nadav Amit ha scritto: >> Looks good. I’ll give it a try but it is hard to give a definitive >> answer, since the emulator is still bug-ridden. > > Yes, we need to write unit tests for this, especially the conforming > case. A bit of a pain to get kvm-unit-tests in ring 3 (access.flat > does it), but I'll give it a shot. > > Paolo > I think the problem might be even more fundamental. According to the SDM, the privilege level checks (CPL/DPL/RPL) are only performed when the segment is loaded; I see no reference to privilege checks when data is accessed. You should be able to load a segment with DPL=0 while you are in CPL=0, then change CPL to 3 and still access the segment (obviously, it is not the best practice). In that case, all the privilege checks in __linearize are redundant and for some extent incorrect. Obviously, I am afraid to submit a patch that removes them, since if the privilege checks of __linearize are needed in certain case, this may introduce security problem. Do you agree? Nadav -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Il 13/10/2014 01:15, Nadav Amit ha scritto: > I think the problem might be even more fundamental. According to the > SDM, the privilege level checks (CPL/DPL/RPL) are only performed when > the segment is loaded; I see no reference to privilege checks when > data is accessed. You should be able to load a segment with DPL=0 > while you are in CPL=0, then change CPL to 3 and still access the > segment (obviously, it is not the best practice). This can be tested without invoking the emulator... Paolo -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Oct 13, 2014 at 02:15:43AM +0300, Nadav Amit wrote: > > > On 10/12/14 3:12 PM, Paolo Bonzini wrote: > > Il 12/10/2014 08:57, Nadav Amit ha scritto: > >> Looks good. I’ll give it a try but it is hard to give a definitive > >> answer, since the emulator is still bug-ridden. > > > > Yes, we need to write unit tests for this, especially the conforming > > case. A bit of a pain to get kvm-unit-tests in ring 3 (access.flat > > does it), but I'll give it a shot. > > > > Paolo > > > > I think the problem might be even more fundamental. > According to the SDM, the privilege level checks (CPL/DPL/RPL) are only performed when the segment is loaded; I see no reference to privilege checks when data is accessed. > You should be able to load a segment with DPL=0 while you are in CPL=0, then change CPL to 3 and still access the segment (obviously, it is not the best practice). > > In that case, all the privilege checks in __linearize are redundant and for some extent incorrect. > Obviously, I am afraid to submit a patch that removes them, since if the privilege checks of __linearize are needed in certain case, this may introduce security problem. > > Do you agree? > 3a78a4f46302bfc83602a53dfa4dcbe76a7a1f5f removed RPL check from __linearize already, so you are probably right, but better verify it on real HW. -- Gleb. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> On Oct 13, 2014, at 2:31 PM, Gleb Natapov <gleb@kernel.org> wrote: > > On Mon, Oct 13, 2014 at 02:15:43AM +0300, Nadav Amit wrote: >> >> >> On 10/12/14 3:12 PM, Paolo Bonzini wrote: >>> Il 12/10/2014 08:57, Nadav Amit ha scritto: >>>> Looks good. I’ll give it a try but it is hard to give a definitive >>>> answer, since the emulator is still bug-ridden. >>> >>> Yes, we need to write unit tests for this, especially the conforming >>> case. A bit of a pain to get kvm-unit-tests in ring 3 (access.flat >>> does it), but I'll give it a shot. >>> >>> Paolo >>> >> >> I think the problem might be even more fundamental. >> According to the SDM, the privilege level checks (CPL/DPL/RPL) are only performed when the segment is loaded; I see no reference to privilege checks when data is accessed. >> You should be able to load a segment with DPL=0 while you are in CPL=0, then change CPL to 3 and still access the segment (obviously, it is not the best practice). >> >> In that case, all the privilege checks in __linearize are redundant and for some extent incorrect. >> Obviously, I am afraid to submit a patch that removes them, since if the privilege checks of __linearize are needed in certain case, this may introduce security problem. >> >> Do you agree? >> > 3a78a4f46302bfc83602a53dfa4dcbe76a7a1f5f removed RPL check from __linearize already, so > you are probably right, but better verify it on real HW. It turns far-ret cannot be used for such experiments, since it rechecks privilege and sets the segments which should be inaccessible (whose DPL is less than the new CPL) to NULL - see Intel SDM "5.8.6 Returning from a Called Procedure”. Using the sysexit, however, I managed to verify the behaviour - DS was loaded with a segment whose DPL=0, then sysexit was executed, and eventually DS was used to access memory (while CPL=3). Experiments were done using the kvm-unit-test environment; the memory accessing instruction was _not_ emulated. Accordingly, I’ll create a new version of this patch which removes all segment privilege checks in __linearize. Nadav-- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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;