From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id B2CE7C433F5 for ; Wed, 9 Feb 2022 22:07:20 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S234986AbiBIWHQ (ORCPT ); Wed, 9 Feb 2022 17:07:16 -0500 Received: from gmail-smtp-in.l.google.com ([23.128.96.19]:56880 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230482AbiBIWHO (ORCPT ); Wed, 9 Feb 2022 17:07:14 -0500 Received: from mail-pf1-x434.google.com (mail-pf1-x434.google.com [IPv6:2607:f8b0:4864:20::434]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 473C2DF28A5A for ; Wed, 9 Feb 2022 14:07:16 -0800 (PST) Received: by mail-pf1-x434.google.com with SMTP id i21so5302595pfd.13 for ; Wed, 09 Feb 2022 14:07:16 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:content-transfer-encoding:in-reply-to; bh=BPvAh/EodqWOHe2tNUbnx1ARgdgM6FUi8nnsTpH0NdI=; b=FycwgfLbLQ6uVp6WdK4GFvG2K1w4g3O8BXx6B/JDBSHIot956M9guxwOz4QQIV4mPE zSqQTJBqcIvEHzUQ+G/woBxkHb+3FohvIATYtIMFKC7ynil85qQ7GyB+nLcQT0kQcFPz lS/KKDzWOC2DT2gAm5uxRliL6GdpAaRKh/aBBJ7r0bHKqhIxPB+n467ihKh10uCVXZn4 uhuzz13axV/2wg9Tp+Jin/DLPe5RLElJb7NgDfaoQgXY3BufTogI7ezpLoAlFyleoXav hPvwnaxkg2eb9uJyIuyYfMVq6Oiv7TY2v1nm5VvBZK9ecAa9OEiVe2Ji172jPivcnAZi MpKQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:content-transfer-encoding :in-reply-to; bh=BPvAh/EodqWOHe2tNUbnx1ARgdgM6FUi8nnsTpH0NdI=; b=wLE4yaCj2os+nnZGsOmMJ+F2kUMprTeISrBBUbQxXwvR7TYjR9QQldr++TsaI0/RFw xp3tT9EcGanTiFCqA80sOaBiPx/m+QzF0ub85IAo8J7ku981E4zOiRl/O4gr70lyYnxz r52hJNiNuOAp4zxhJJcSHlFy5XhglYIQhfcwMBztAvaV+DK4gQJHwNjtUnMHi7Fg8FrS m/osUuonGwZo5LgyoSbrha0j2l2dX+bzX+V07fgR/XRGYGUkPxvTl33z3vU0Cw8CRVww TveJEIEsI0Dexb6VgiSkfLWKBp/og1cvJCmqD7XGlYy20dmF9b7zvQyAYqwzNRrhJ8oh Q3Ew== X-Gm-Message-State: AOAM531Zmj4WwGZStGURdl0hy7DKYxUjvzC+Tdc455lgUXCHlt5AYpmY 9nYXtDX8Oh6iEVTieoLWv3wtxA== X-Google-Smtp-Source: ABdhPJwXjScvzlrmXXZn4qPmhKJVfITTp8bWYIkO4RwYLE5hfnD11dFed5b6MnSqdgiSUbCh0Lr2Pg== X-Received: by 2002:a63:f34e:: with SMTP id t14mr3645452pgj.622.1644444435507; Wed, 09 Feb 2022 14:07:15 -0800 (PST) Received: from google.com (157.214.185.35.bc.googleusercontent.com. [35.185.214.157]) by smtp.gmail.com with ESMTPSA id j3sm15288799pgs.0.2022.02.09.14.07.14 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 09 Feb 2022 14:07:14 -0800 (PST) Date: Wed, 9 Feb 2022 22:07:11 +0000 From: Sean Christopherson To: Hou Wenlong Cc: kvm@vger.kernel.org, Paolo Bonzini Subject: Re: [kvm-unit-tests PATCH v2 1/2] x86/emulator: Add some tests for lret instruction emulation Message-ID: References: <006c75f53539958c1e5d5a0a5073566a5395414e.1644311445.git.houwenlong.hwl@antgroup.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <006c75f53539958c1e5d5a0a5073566a5395414e.1644311445.git.houwenlong.hwl@antgroup.com> Precedence: bulk List-ID: X-Mailing-List: kvm@vger.kernel.org Nit, "far ret" instead of "lret" in the shortlog. On Tue, Feb 08, 2022, Hou Wenlong wrote: > Per Intel's SDM on the "Instruction Set Reference", when > loading segment descriptor for far return, not-present segment > check should be after all type and privilege checks. However, > __load_segment_descriptor() in x86's emulator does not-present > segment check first, so it would trigger #NP instead of #GP > if type or privilege checks fail and the segment is not present. > > And if RPL < CPL, it should trigger #GP, but the check is missing > in emulator. > > So add some tests for lret instruction, and it will test > those tests in hardware and emulator. Enable > kvm.force_emulation_prefix when try to test them in emulator. > > Signed-off-by: Hou Wenlong > --- Was this tested? There are multiple compilation errors with gcc... x86/emulator.c: In function ‘test_far_xfer’: x86/emulator.c:1034:10: error: format not a string literal and no format arguments [-Werror=format-security] 1034 | far_xfer_error_code == t->error_code, t->msg); | ^~~~~~~~~~~~~~~~~~~ cc1: all warnings being treated as errors make: *** [: x86/emulator.o] Error 1 x86/emulator.c: Assembler messages: x86/emulator.c:81: Error: incorrect register `%si' used with `q' suffix x86/emulator.c:83: Error: incorrect register `%si' used with `q' suffix make: *** [: x86/emulator.o] Error 1 > +static struct far_xfer_test_case far_ret_testcases[] = { > + {0, DS_TYPE, 0, 0, false, GP_VECTOR, FIRST_SPARE_SEL, "lret desc.type!=code && desc.p=0"}, > + {0, NON_CONFORM_CS_TYPE, 3, 0, false, GP_VECTOR, FIRST_SPARE_SEL, "lret non-conforming && dpl!=rpl && desc.p=0"}, > + {0, CONFORM_CS_TYPE, 3, 0, false, GP_VECTOR, FIRST_SPARE_SEL, "lret conforming && dpl>rpl && desc.p=0"}, > + {0, NON_CONFORM_CS_TYPE, 0, 0, false, NP_VECTOR, FIRST_SPARE_SEL, "lret desc.p=0"}, > + {0, NON_CONFORM_CS_TYPE, 3, 1, true, GP_VECTOR, FIRST_SPARE_SEL, "lret rpl +}; > + > +static struct far_xfer_test far_ret_test = { > + .insn = FAR_XFER_RET, > + .testcases = &far_ret_testcases[0], > + .nr_testcases = sizeof(far_ret_testcases) / sizeof(struct far_xfer_test_case), > +}; > + > +#define TEST_FAR_RET_ASM(seg, prefix) \ > + asm volatile("lea 1f(%%rip), %%rax\n\t" \ > + "pushq %[asm_seg]\n\t" \ > + "pushq $2f\n\t" \ > + prefix "lretq\n\t" \ > + "1: addq $16, %%rsp\n\t" \ > + "2:" \ > + : : [asm_seg]"r"(seg) \ Because @seg is a u16, gcc emites "pushq SI". Kinda dumb, but... @@ -72,7 +71,7 @@ static struct far_xfer_test far_ret_test = { prefix "lretq\n\t" \ "1: addq $16, %%rsp\n\t" \ "2:" \ - : : [asm_seg]"r"(seg) \ + : : [asm_seg]"r"((u64)seg) \ : "eax", "memory"); > + : "eax", "memory"); > + > +static inline void test_far_ret_asm(uint16_t seg, bool force_emulation) > +{ > + if (force_emulation) { No need for braces. Ah, the macro is missing ({ ... }) to force it to be evaluated as a single statement. It should be. Side topic, a ternary operator would be nice, but I don't know how to force string concatentation for this case... #define TEST_FAR_RET_ASM(seg, prefix) \ ({ \ asm volatile("lea 1f(%%rip), %%rax\n\t" \ "pushq %[asm_seg]\n\t" \ "pushq $2f\n\t" \ prefix "lretq\n\t" \ "1: addq $16, %%rsp\n\t" \ "2:" \ : : [asm_seg]"r"((u64)seg) \ : "eax", "memory"); \ }) > + TEST_FAR_RET_ASM(seg, KVM_FEP); > + } else { > + TEST_FAR_RET_ASM(seg, ""); > + } > +} This only has a single caller, just open code the macros in the case statements of __test_far_xfer(). (My apologies if I suggested this in the last version). > struct regs { > u64 rax, rbx, rcx, rdx; > @@ -891,6 +951,74 @@ static void test_mov_dr(uint64_t *mem) > report(rax == dr6_fixed_1, "mov_dr6"); > } > > +static void far_xfer_exception_handler(struct ex_regs *regs) > +{ > + far_xfer_vector = regs->vector; > + far_xfer_error_code = regs->error_code; > + regs->rip = regs->rax;; Double semi-colon. > +} > + > +static void __test_far_xfer(enum far_xfer_insn insn, uint16_t seg, > + bool force_emulation) > +{ > + switch (insn) { > + case FAR_XFER_RET: > + test_far_ret_asm(seg, force_emulation); > + break; > + default: > + report_fail("unknown instructions"); It's worth spitting out the insn, it might save someone a few seconds/minutes, e.g. report_fail("Unexpected insn enum = %d\n", insn); > + break; > + } > +} > + > +static void test_far_xfer(bool force_emulation, struct far_xfer_test *test) > +{ > + struct far_xfer_test_case *t; > + uint16_t seg; > + bool ign; > + int i; > + > + handle_exception(GP_VECTOR, far_xfer_exception_handler); > + handle_exception(NP_VECTOR, far_xfer_exception_handler); > + > + for (i = 0; i < test->nr_testcases; i++) { > + t = &test->testcases[i]; > + > + seg = FIRST_SPARE_SEL | t->rpl; > + gdt[seg / 8] = gdt[(t->usermode ? USER_CS64 : KERNEL_CS) / 8]; > + gdt[seg / 8].type = t->type; > + gdt[seg / 8].dpl = t->dpl; > + gdt[seg / 8].p = t->p; > + > + far_xfer_vector = -1; > + far_xfer_error_code = -1; > + > + if (t->usermode) > + run_in_user((usermode_func)__test_far_xfer, UD_VECTOR, > + test->insn, seg, force_emulation, 0, &ign); > + else > + __test_far_xfer(test->insn, seg, force_emulation); > + > + report(far_xfer_vector == t->vector && > + far_xfer_error_code == t->error_code, t->msg); To avoid having to specify the error code, just do: (far_xfer_vector < 0 || far_xfer_error_code == FIRST_SPARE_SEL), far_xfer_vector and t->vector need to be signed ints, but that's perfectly ok, vector fits in a u8. Printing out the expecte vs. actual is also helpful for debug (pet peeve of mine in KUT...). I doesn't have to be super fancy formatting, just enough so that the user doesn't have to modify the test to figure out what went wrong. And with the insn_name + msg + target (see below) formatting: report(far_xfer_vector == t->vector && (far_xfer_vector < 0 || far_xfer_error_code == FIRST_SPARE_SEL), "%s on %s, %s: wanted %d (%d), got %d (%d)", test->insn_name, force_emulation ? "emuator" : "hardware", t->msg, t->vector, t->vector < 0 ? -1 : FIRST_SPARE_SEL, far_xfer_vector, far_xfer_error_code); > + } > + > + handle_exception(GP_VECTOR, 0); > + handle_exception(NP_VECTOR, 0); > +} > + > +static void test_lret(uint64_t *mem) test_far_ret() > +{ > + printf("test lret in hw\n"); Rather than print this out before, just spit out the "target" in the report. > + test_far_xfer(false, &far_ret_test); > +} All of the above (I think) feeback: --- x86/emulator.c | 57 +++++++++++++++++++++++++------------------------- 1 file changed, 28 insertions(+), 29 deletions(-) diff --git a/x86/emulator.c b/x86/emulator.c index 2a35caf..d28e36c 100644 --- a/x86/emulator.c +++ b/x86/emulator.c @@ -21,7 +21,7 @@ static int exceptions; #define KVM_FEP "ud2; .byte 'k', 'v', 'm';" #define KVM_FEP_LENGTH 5 static int fep_available = 1; -static unsigned int far_xfer_vector = -1; +static int far_xfer_vector = -1; static unsigned int far_xfer_error_code = -1; struct far_xfer_test_case { @@ -30,8 +30,7 @@ struct far_xfer_test_case { uint16_t dpl; uint16_t p; bool usermode; - unsigned int vector; - unsigned int error_code; + int vector; const char *msg; }; @@ -41,6 +40,7 @@ enum far_xfer_insn { struct far_xfer_test { enum far_xfer_insn insn; + const char *insn_name; struct far_xfer_test_case *testcases; unsigned int nr_testcases; }; @@ -50,37 +50,31 @@ struct far_xfer_test { #define DS_TYPE 0x3 static struct far_xfer_test_case far_ret_testcases[] = { - {0, DS_TYPE, 0, 0, false, GP_VECTOR, FIRST_SPARE_SEL, "lret desc.type!=code && desc.p=0"}, - {0, NON_CONFORM_CS_TYPE, 3, 0, false, GP_VECTOR, FIRST_SPARE_SEL, "lret non-conforming && dpl!=rpl && desc.p=0"}, - {0, CONFORM_CS_TYPE, 3, 0, false, GP_VECTOR, FIRST_SPARE_SEL, "lret conforming && dpl>rpl && desc.p=0"}, - {0, NON_CONFORM_CS_TYPE, 0, 0, false, NP_VECTOR, FIRST_SPARE_SEL, "lret desc.p=0"}, - {0, NON_CONFORM_CS_TYPE, 3, 1, true, GP_VECTOR, FIRST_SPARE_SEL, "lret rplrpl && desc.p=0"}, + {0, NON_CONFORM_CS_TYPE, 0, 0, false, NP_VECTOR, "desc.p=0"}, + {0, NON_CONFORM_CS_TYPE, 3, 1, true, GP_VECTOR, "rplvector; far_xfer_error_code = regs->error_code; - regs->rip = regs->rax;; + regs->rip = regs->rax; } static void __test_far_xfer(enum far_xfer_insn insn, uint16_t seg, @@ -967,10 +961,13 @@ static void __test_far_xfer(enum far_xfer_insn insn, uint16_t seg, { switch (insn) { case FAR_XFER_RET: - test_far_ret_asm(seg, force_emulation); + if (force_emulation) + TEST_FAR_RET_ASM(seg, KVM_FEP); + else + TEST_FAR_RET_ASM(seg, ""); break; default: - report_fail("unknown instructions"); + report_fail("Unexpected insn enum = %d\n", insn); break; } } @@ -1004,22 +1001,24 @@ static void test_far_xfer(bool force_emulation, struct far_xfer_test *test) __test_far_xfer(test->insn, seg, force_emulation); report(far_xfer_vector == t->vector && - far_xfer_error_code == t->error_code, "%s", t->msg); + (far_xfer_vector < 0 || far_xfer_error_code == FIRST_SPARE_SEL), + "%s on %s, %s: wanted %d (%d), got %d (%d)", + test->insn_name, force_emulation ? "emuator" : "hardware", t->msg, + t->vector, t->vector < 0 ? -1 : FIRST_SPARE_SEL, + far_xfer_vector, far_xfer_error_code); } handle_exception(GP_VECTOR, 0); handle_exception(NP_VECTOR, 0); } -static void test_lret(uint64_t *mem) +static void test_far_ret(uint64_t *mem) { - printf("test lret in hw\n"); test_far_xfer(false, &far_ret_test); } -static void test_em_lret(uint64_t *mem) +static void test_em_far_ret(uint64_t *mem) { - printf("test lret in emulator\n"); test_far_xfer(true, &far_ret_test); } @@ -1297,7 +1296,7 @@ int main(void) test_smsw(mem); test_lmsw(); test_ljmp(mem); - test_lret(mem); + test_far_ret(mem); test_stringio(); test_incdecnotneg(mem); test_btc(mem); @@ -1322,7 +1321,7 @@ int main(void) test_smsw_reg(mem); test_nop(mem); test_mov_dr(mem); - test_em_lret(mem); + test_em_far_ret(mem); } else { report_skip("skipping register-only tests, " "use kvm.force_emulation_prefix=1 to enable"); base-commit: b56a1a58abfcc6abc16e782f13505f4495cf59e8 --