[LibOS] Emulate in/out instructions as if they generate SIGSEGV #1192
Conversation
There was a problem hiding this comment.
Reviewable status: 0 of 6 files reviewed, 11 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @NirjharRoyiitk)
-- commits line 7 at r1:
The two commits should be merged into one. There is no sense in splitting them.
-- commits line 7 at r1:
Also, please check out how we format commit message titles and bodies (just check the history of commits), and please apply the same rules here.
Feel free to force-push the changes, since noone is actively reviewing this PR yet.
libos/src/bookkeep/libos_signal.c line 503 at r1 (raw file):
* https://c9x.me/x86/html/file_module_x86_id_222.html (for OUT instruction) * * Some other releavant links
I would remove all these "relevant links", they don't deserve to be in the codebase. Instead, you can put them as one of the Reviewable comments, or even just add them to the PR description.
libos/src/bookkeep/libos_signal.c line 525 at r1 (raw file):
log_debug("Illegal instruction (or legal but unsupported instruction "\ "inside enclave) during app execution at %p; delivering to app", rip);
Please never leave trailing whitespaces in PRs.
libos/test/fs/lscpu-test.c line 17 at r1 (raw file):
#define EXPECTED_NUM_SIGSEGVS 2 #define COMMAND "/usr/bin/lscpu"
Please remove the sub-test for lscpu from this test. This command may not be installed on many systems, and to test your added functionality, it is enough to have the synthetic in/out assembly snippets.
libos/test/fs/lscpu-test.c line 24 at r1 (raw file):
static void fault_handler(int ignored) {
Please read our coding guidelines: https://gramine.readthedocs.io/en/stable/devel/coding-style.html
libos/test/fs/lscpu-test.c line 26 at r1 (raw file):
{ g_sigsegv_triggered++; siglongjmp(g_point, 1);
We'll need to review the use of longjmp here, I'm not a fan (and it's probably a UD?)
libos/test/fs/lscpu-test.c line 58 at r1 (raw file):
assert(ret == 0); printf("\nlscpu-test test passed\n");
Please check other tests, that's not how we print success.
libos/test/fs/lscpu-test.manifest.template line 1 at r1 (raw file):
loader.entrypoint = "file:{{ gramine.libos }}"
Why do you need a separate manifest for this test? Because of lscpu? I suggest to remove lscpu from this test completely, and instead just use the generic manifest.template (i.e., you won't need to add anything special to this PR)
libos/test/fs/test_lscpu.py line 11 at r1 (raw file):
def setUp(self): pass
Why do you need these setUpClass and setUp?
Generally, please remove lscpu program from your test, and put this test under some already-existing class here.
libos/test/fs/tests.toml line 21 at r1 (raw file):
"stat", "truncate", "lscpu-test"
You'll need to also add the test to tests-musl.toml
NirjharRoyiitk
force-pushed
the
NirjharRoyiitk/issue-1191-in-out-instruction-throwing-SIGILL
branch
from
d40594f
to
f436c12
Compare
There was a problem hiding this comment.
Reviewable status: 0 of 7 files reviewed, 11 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ), period found at the end of a one-liner (waiting on @dimakuv)
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
The two commits should be merged into one. There is no sense in splitting them.
Done.
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Also, please check out how we format commit message titles and bodies (just check the history of commits), and please apply the same rules here.
Feel free to force-push the changes, since noone is actively reviewing this PR yet.
Done.
libos/src/bookkeep/libos_signal.c line 503 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
I would remove all these "relevant links", they don't deserve to be in the codebase. Instead, you can put them as one of the Reviewable comments, or even just add them to the PR description.
Done.
libos/src/bookkeep/libos_signal.c line 525 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Please never leave trailing whitespaces in PRs.
Done.
libos/test/fs/tests.toml line 21 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
You'll need to also add the test to
tests-musl.toml
This wasn't required for my test.
libos/test/fs/lscpu-test.manifest.template line 1 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Why do you need a separate manifest for this test? Because of
lscpu? I suggest to removelscpufrom this test completely, and instead just use the genericmanifest.template(i.e., you won't need to add anything special to this PR)
Done.
libos/test/fs/test_lscpu.py line 11 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Why do you need these
setUpClassandsetUp?Generally, please remove
lscpuprogram from your test, and put this test under some already-existing class here.
Done.
libos/test/fs/lscpu-test.c line 17 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Please remove the sub-test for
lscpufrom this test. This command may not be installed on many systems, and to test your added functionality, it is enough to have the syntheticin/outassembly snippets.
Done.
libos/test/fs/lscpu-test.c line 24 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Please read our coding guidelines: https://gramine.readthedocs.io/en/stable/devel/coding-style.html
Done.
libos/test/fs/lscpu-test.c line 58 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Please check other tests, that's not how we print success.
Done.
libos/test/fs/tests.toml
Outdated
| @@ -18,4 +18,5 @@ manifests = [ | |||
| "seek_tell_truncate", | |||
| "stat", | |||
| "truncate", | |||
| "in_out_instruction_test" | |||
There was a problem hiding this comment.
The new test should probably be inserted in alphabetical order.
libos/src/bookkeep/libos_signal.c
Outdated
| * opcode for in or out instruction. | ||
| */ | ||
| static bool is_in_out(PAL_CONTEXT* context) { | ||
| unsigned char opcodes[] = {0xe4, 0xe5, 0xec, 0xed, 0xe6, 0xe7, |
There was a problem hiding this comment.
I think it's worth having comments to explain which instruction each opcode corresponds to.
libos/src/bookkeep/libos_signal.c
Outdated
| unsigned char opcodes[] = {0xe4, 0xe5, 0xec, 0xed, 0xe6, 0xe7, | ||
| 0xee, 0xef, 0x6c, 0x6d, 0x6e, 0x6f}; | ||
| int num_opcodes = sizeof(opcodes)/sizeof(opcodes[0]); | ||
| uint8_t* rip = (uint8_t*)context->rip; |
There was a problem hiding this comment.
I don't think the check here doesn't cover all possible encodings of these instructions. x86 typically allows things like segment overrides and opsize and code size overrides, even if they don't make sense for the instruction being encoded.
There was a problem hiding this comment.
Reviewable status: 0 of 7 files reviewed, 14 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ), period found at the end of a one-liner, "fixup! " found in commit messages' one-liners (waiting on @arai-fortanix and @dimakuv)
libos/src/bookkeep/libos_signal.c line 468 at r2 (raw file):
Previously, arai-fortanix (Daniel Arai) wrote…
I think it's worth having comments to explain which instruction each opcode corresponds to.
Done.
libos/src/bookkeep/libos_signal.c line 471 at r2 (raw file):
Previously, arai-fortanix (Daniel Arai) wrote…
I don't think the check here doesn't cover all possible encodings of these instructions. x86 typically allows things like segment overrides and opsize and code size overrides, even if they don't make sense for the instruction being encoded.
Done. Took the prefix values from https://wiki.osdev.org/X86-64_Instruction_Encoding
libos/test/fs/tests.toml line 21 at r2 (raw file):
Previously, arai-fortanix (Daniel Arai) wrote…
The new test should probably be inserted in alphabetical order.
Done.
NirjharRoyiitk
force-pushed
the
NirjharRoyiitk/issue-1191-in-out-instruction-throwing-SIGILL
branch
from
34fbbcb
to
b11caad
Compare
There was a problem hiding this comment.
Reviewed 4 of 7 files at r2, 2 of 3 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 33 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ), period found at the end of a one-liner, "fixup! " found in commit messages' one-liners (waiting on @arai-fortanix and @NirjharRoyiitk)
Previously, NirjharRoyiitk wrote…
Done.
Still wrong:
- Should be
[LibOS] - The title is too long and doesn't start with a verb
I would suggest like this:
[LibOS] Emulate in/out instructions as if they generate SIGSEGV
Executing I/O instructions (e.g., in/out) inside an SGX enclave generates
a #UD fault. Gramine's PAL tries to handle this exception and propogates
it to LibOS/app as a SIGILL signal.
However, I/O instructions result in a #GP fault (which raises a SIGSEGV
signal) if I/O is not permitted. Let Gramine emulate these instructions as
if they end up in SIGSEGV. This helps some apps, e.g. `lscpu`.
libos/src/bookkeep/libos_signal.c line 465 at r4 (raw file):
uint8_t prefix_list[] = { /* Group 0 */ 0xf0, /* LOCK prefix */
LOCK prefix is not allowed with IN/OUT/INS/OUTS instructions, so it's meaningless to check it here
libos/src/bookkeep/libos_signal.c line 476 at r4 (raw file):
0x65, /* GS segment override */ 0x2e, /* Branch not taken */ 0x3e, /* Branch taken */
I think this whole Group 1 of prefixes is useless for 64-bit IN/OUT/... instructions?
libos/src/bookkeep/libos_signal.c line 484 at r4 (raw file):
* for the instruction(s) we are checking. */ };
Out of all these, I think only REP and 0x66 and 0x67 prefixes make sense with IN/OUT instructions
libos/src/bookkeep/libos_signal.c line 486 at r4 (raw file):
}; size_t num_prefixes = sizeof(prefix_list)/sizeof(*prefix_list); for(int i = 0; i < num_prefixes; i++) {
In Gramine, for such cases we use size_t instead of int
libos/src/bookkeep/libos_signal.c line 486 at r4 (raw file):
}; size_t num_prefixes = sizeof(prefix_list)/sizeof(*prefix_list); for(int i = 0; i < num_prefixes; i++) {
We have a 100 char limit, so you can remove helper var num_prefixes and just embed the equation
libos/src/bookkeep/libos_signal.c line 497 at r4 (raw file):
* This function checks if the rip on which the exception is occuring is a valid * opcode for in or out instruction. */
I would remove this whole comment -- the func is self explanatory.
libos/src/bookkeep/libos_signal.c line 499 at r4 (raw file):
*/ static bool is_in_out(PAL_CONTEXT* context) { unsigned char opcodes[] = {
uint8_t please
libos/src/bookkeep/libos_signal.c line 509 at r4 (raw file):
0xe7, /* OUT imm8, AX/EAX */ 0xee, /* OUT DX, AL */ 0xef, /* OUT DX, AX */
I would also add INS/... family of instructions, and OUTS/... family of instructions in this list.
libos/src/bookkeep/libos_signal.c line 516 at r4 (raw file):
/* num_prefixes_found will store the actual opcode index in rip */ size_t num_prefixes_found = 0; for(int i = 0; i < num_prefixes_to_check; i++) {
Please add a space after for
Also, please use size_t
libos/src/bookkeep/libos_signal.c line 522 at r4 (raw file):
break; } assert(num_prefixes_found <= num_prefixes_to_check);
This assert is redundant -- the logic is 5 lines of trivial code, so it's clear this condition is always true.
libos/src/bookkeep/libos_signal.c line 523 at r4 (raw file):
} assert(num_prefixes_found <= num_prefixes_to_check); for(int i = 0; i < num_opcodes; i++) {
ditto
libos/src/bookkeep/libos_signal.c line 548 at r4 (raw file):
/* Emulate syscall instruction, which is prohibited in Linux-SGX PAL and raises a SIGILL. */ if (!maybe_emulate_syscall(context)) { /* IN/OUT instruction aren't allowed in SGX enclaves. Usually #GP hardware exception
Please create a separate function for all this new code, called bool maybe_raise_sigsegv(). I can imagine that we'll have more than IN/OUT instructions with similar behavior, so it makes sense to have a more generic func for this behavior.
Basically, this func will be a wrapper around your current is_in_out().
libos/src/bookkeep/libos_signal.c line 553 at r4 (raw file):
* that tries to get various information about cpus. This was required * in some tensorflow application and that made it necessary to support * lscpu.
I suggest to change this comment to the text that I proposed in the commit body message
libos/src/bookkeep/libos_signal.c line 563 at r4 (raw file):
signo = SIGILL; si_code = ILL_ILLOPC; }
That's too much code, it could be written in a nicer way:
siginfo_t info = {
.si_signo = SIGILL,
.si_code = ILL_ILLOPC,
.si_addr = (void*)addr,
};
if (maybe_raise_sigsegv()) {
info.si_signo = SIGSEGV;
info.si_code = SEGV_MAPERR;
log_debug("Illegal instruction during app execution at %p, emulated as if throwing SIGSEGV;"
" delivering to app", rip);
} else {
log_debug("Illegal instruction during app execution at %p; delivering to app", rip);
}
libos/src/bookkeep/libos_signal.c line 566 at r4 (raw file):
void* rip = (void*)pal_context_get_ip(context); log_debug("Illegal instruction (or legal but unsupported instruction "\ "inside enclave) during app execution at %p; delivering to app", rip);
I don't like this change to the log message. Please see my proposal in other comment.
libos/test/fs/meson.build line 43 at r4 (raw file):
'link_args': '-lpthread', }, 'in_out_instruction_test': {},
Why is this test located under fs/? It has nothing to do with files.
Please move the test to under libos/test/regression/
libos/test/fs/tests.toml line 21 at r1 (raw file):
Previously, NirjharRoyiitk wrote…
This wasn't required for my test.
I don't get it. I guess it's because you created the test under fs/, but it should have been under regression/ (which has this tests-musl.toml file)
libos/test/fs/in_out_instruction_test.c line 3 at r4 (raw file):
/* Test Description: This test verifies that in/out instructions correctly generate * SIGSEGV. It raises SIGSEGV once for in and one for out and then counts if * number of SIGSEGVs are is 2.
Could you please beautify this comment? You have typos (are is), unwarranted capital letters, trailing whitespace, and seems like the char limit of 100 is not correctly set.
libos/test/fs/in_out_instruction_test.c line 8 at r4 (raw file):
#define _XOPEN_SOURCE 700 #define _POSIX_C_SOURCE 200809
Why are these defines needed?
libos/test/fs/in_out_instruction_test.c line 19 at r4 (raw file):
int g_sigsegv_triggered = 0; jmp_buf g_point; jmp_buf g_point2;
Not used?
libos/test/fs/in_out_instruction_test.c line 21 at r4 (raw file):
jmp_buf g_point2; static void fault_handler(int ignored) {
I would actually rename ignored -> signum and double-check (or at least assert) that it is SIGSEGV.
libos/test/fs/in_out_instruction_test.c line 30 at r4 (raw file):
struct sigaction int_handler = {.sa_handler=fault_handler, .sa_flags = SA_RESTART}; sigaction(SIGSEGV, &int_handler, 0);
TODO to myself: check that this usage is legit in signal handlers.
libos/test/fs/in_out_instruction_test.c line 32 at r4 (raw file):
sigaction(SIGSEGV, &int_handler, 0); if (sigsetjmp(g_point, 1) == 0) { __asm__("in %dx, %al;");/* AT & T style */
We should assign some (meaningful) values in input regs (RDX, RAX) here, otherwise it's totally bullocks.
libos/test/fs/in_out_instruction_test.c line 34 at r4 (raw file):
__asm__("in %dx, %al;");/* AT & T style */ } printf("handled in instruction\n");
In such no-variable cases, please use puts()
libos/test/fs/in_out_instruction_test.c line 37 at r4 (raw file):
if (sigsetjmp(g_point, 1) == 0) { __asm__("out %al, %dx;");/* AT & T style */
We should assign some (meaningful) values in input regs (RDX, RAX) here, otherwise it's totally bullocks.
libos/test/fs/in_out_instruction_test.c line 39 at r4 (raw file):
__asm__("out %al, %dx;");/* AT & T style */ } printf("handled out instruction\n");
In such no-variable cases, please use puts()
libos/test/fs/in_out_instruction_test.c line 41 at r4 (raw file):
printf("handled out instruction\n"); assert(g_sigsegv_triggered == EXPECTED_NUM_SIGSEGVS);
Still some whitespaces here and there in your PR...
libos/test/fs/lscpu-test.c line 58 at r1 (raw file):
Previously, NirjharRoyiitk wrote…
Done.
Let's just print TEST OK, the current text reads like broken English to me.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 38 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ), period found at the end of a one-liner, "fixup! " found in commit messages' one-liners (waiting on @arai-fortanix, @dimakuv, and @NirjharRoyiitk)
libos/src/bookkeep/libos_signal.c line 462 at r4 (raw file):
} static bool is_byte_prefix(uint8_t op) {
I would call the current version is_legacy_prefix()
Code quote:
is_byte_prefixlibos/src/bookkeep/libos_signal.c line 465 at r4 (raw file):
LOCK prefix is not allowed with IN/OUT/INS/OUTS instructions
+1
libos/src/bookkeep/libos_signal.c line 476 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
I think this whole Group 1 of prefixes is useless for 64-bit IN/OUT/... instructions?
Not really, I would say the branch hints prefixes do not make sense but may not for the segment override prefixes.
BTW, if we change the list, then we intend to implement this function non-generically (i.e., specific for IN/OUT now).
libos/src/bookkeep/libos_signal.c line 484 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Out of all these, I think only REP and 0x66 and 0x67 prefixes make sense with IN/OUT instructions
Also REPNx and see my comments above.
libos/src/bookkeep/libos_signal.c line 497 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
I would remove this whole comment -- the func is self explanatory.
+1
libos/src/bookkeep/libos_signal.c line 500 at r4 (raw file):
static bool is_in_out(PAL_CONTEXT* context) { unsigned char opcodes[] = { /*imm8 is an immediate value denoting port address */
Suggest to have sth like the following where the instruction opcodes are ordered and grouped:
/* INS opcodes */
0x6c,
0x6d,
/* OUTS opcodes */
0x6e,
0x6f,
/* IN immediate opcodes */
0xe4,
0xe5,
/* OUT immediate opcodes */
0xe6,
0xe7,
/* IN register opcodes */
0xec,
0xed,
/* OUT register opcodes */
0xee,
0xef,
BTW IMO, no need to specify the exact instruction(s) for each like IN AX/EAX,imm8.
libos/src/bookkeep/libos_signal.c line 548 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Please create a separate function for all this new code, called
bool maybe_raise_sigsegv(). I can imagine that we'll have more than IN/OUT instructions with similar behavior, so it makes sense to have a more generic func for this behavior.Basically, this func will be a wrapper around your current
is_in_out().
+1
libos/test/fs/in_out_instruction_test.c line 18 at r4 (raw file):
#define EXPECTED_NUM_SIGSEGVS 2 int g_sigsegv_triggered = 0; jmp_buf g_point;
static
libos/test/fs/in_out_instruction_test.c line 30 at r4 (raw file):
struct sigaction int_handler = {.sa_handler=fault_handler, .sa_flags = SA_RESTART}; sigaction(SIGSEGV, &int_handler, 0);
NULL
Code quote:
0libos/test/fs/in_out_instruction_test.c line 31 at r4 (raw file):
.sa_flags = SA_RESTART}; sigaction(SIGSEGV, &int_handler, 0); if (sigsetjmp(g_point, 1) == 0) {
We should do CHECK(sigsetjump()) to catch errors.
Code quote:
sigsetjmp(g_point, 1) == 0libos/test/fs/lscpu-test.c line 26 at r1 (raw file):
I'm not a fan
I'm fine w/ using it in tests.
There was a problem hiding this comment.
Reviewable status: 2 of 12 files reviewed, 38 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @arai-fortanix, @dimakuv, and @kailun-qin)
libos/src/bookkeep/libos_signal.c line 462 at r4 (raw file):
Previously, kailun-qin (Kailun Qin) wrote…
I would call the current version
is_legacy_prefix()
Done.
libos/src/bookkeep/libos_signal.c line 465 at r4 (raw file):
Previously, kailun-qin (Kailun Qin) wrote…
LOCK prefix is not allowed with IN/OUT/INS/OUTS instructions
+1
I kept this list of prefixes to be generic. In case it is required to check it for some other set of instructions and not specifically for IN/OUT family of instructions.
libos/src/bookkeep/libos_signal.c line 476 at r4 (raw file):
Previously, kailun-qin (Kailun Qin) wrote…
Not really, I would say the branch hints prefixes do not make sense but may not for the segment override prefixes.
BTW, if we change the list, then we intend to implement this function non-generically (i.e., specific for
IN/OUTnow).
I kept this list of prefixes to be generic. In case it is required to check it for some other set of instructions and not specifically for IN/OUT family of instructions.
libos/src/bookkeep/libos_signal.c line 484 at r4 (raw file):
Previously, kailun-qin (Kailun Qin) wrote…
Also
REPNxand see my comments above.
I kept this list of prefixes to be generic. In case it is required to check it for some other set of instructions and not specifically for IN/OUT family of instructions.
libos/src/bookkeep/libos_signal.c line 486 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
In Gramine, for such cases we use
size_tinstead ofint
Done.
libos/src/bookkeep/libos_signal.c line 486 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
We have a 100 char limit, so you can remove helper var
num_prefixesand just embed the equation
Done.
libos/src/bookkeep/libos_signal.c line 497 at r4 (raw file):
Previously, kailun-qin (Kailun Qin) wrote…
+1
Done.
libos/src/bookkeep/libos_signal.c line 499 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
uint8_tplease
Done.
libos/src/bookkeep/libos_signal.c line 500 at r4 (raw file):
Previously, kailun-qin (Kailun Qin) wrote…
Suggest to have sth like the following where the instruction opcodes are ordered and grouped:
/* INS opcodes */ 0x6c, 0x6d, /* OUTS opcodes */ 0x6e, 0x6f, /* IN immediate opcodes */ 0xe4, 0xe5, /* OUT immediate opcodes */ 0xe6, 0xe7, /* IN register opcodes */ 0xec, 0xed, /* OUT register opcodes */ 0xee, 0xef,BTW IMO, no need to specify the exact instruction(s) for each like
IN AX/EAX,imm8.
Done.
libos/src/bookkeep/libos_signal.c line 509 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
I would also add INS/... family of instructions, and OUTS/... family of instructions in this list.
Done.
libos/src/bookkeep/libos_signal.c line 516 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Please add a space after
forAlso, please use
size_t
Done.
libos/src/bookkeep/libos_signal.c line 522 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
This assert is redundant -- the logic is 5 lines of trivial code, so it's clear this condition is always true.
Done.
libos/src/bookkeep/libos_signal.c line 523 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
ditto
Done.
libos/src/bookkeep/libos_signal.c line 548 at r4 (raw file):
Previously, kailun-qin (Kailun Qin) wrote…
+1
Done.
libos/src/bookkeep/libos_signal.c line 553 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
I suggest to change this comment to the text that I proposed in the commit body message
Done.
libos/test/fs/meson.build line 43 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Why is this test located under
fs/? It has nothing to do with files.Please move the test to under
libos/test/regression/
Done.
libos/test/fs/tests.toml line 21 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
I don't get it. I guess it's because you created the test under
fs/, but it should have been underregression/(which has thistests-musl.tomlfile)
Done.
libos/test/fs/in_out_instruction_test.c line 3 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Could you please beautify this comment? You have typos (
are is), unwarranted capital letters, trailing whitespace, and seems like the char limit of 100 is not correctly set.
Done.
libos/test/fs/in_out_instruction_test.c line 8 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Why are these defines needed?
Without these, some of the signal related structures and macros are not getting detected during compile time. This happens because Gramine compiles with -std=c11 .
https://stackoverflow.com/questions/6491019/struct-sigaction-incomplete-error
libos/test/fs/in_out_instruction_test.c line 18 at r4 (raw file):
Previously, kailun-qin (Kailun Qin) wrote…
static
Done.
libos/test/fs/in_out_instruction_test.c line 19 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Not used?
removed. Done
libos/test/fs/in_out_instruction_test.c line 21 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
I would actually rename
ignored->signumand double-check (or at least assert) that it is SIGSEGV.
Done.
libos/test/fs/in_out_instruction_test.c line 31 at r4 (raw file):
Previously, kailun-qin (Kailun Qin) wrote…
We should do
CHECK(sigsetjump())to catch errors.
Done.
libos/test/fs/in_out_instruction_test.c line 32 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
We should assign some (meaningful) values in input regs (RDX, RAX) here, otherwise it's totally bullocks.
Done.
libos/test/fs/in_out_instruction_test.c line 34 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
In such no-variable cases, please use
puts()
Done.
libos/test/fs/in_out_instruction_test.c line 37 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
We should assign some (meaningful) values in input regs (RDX, RAX) here, otherwise it's totally bullocks.
Done.
libos/test/fs/in_out_instruction_test.c line 39 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
In such no-variable cases, please use
puts()
Done.
libos/test/fs/in_out_instruction_test.c line 41 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Still some whitespaces here and there in your PR...
Done.
libos/test/fs/lscpu-test.c line 58 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Let's just print
TEST OK, the current text reads like broken English to me.
Done.
There was a problem hiding this comment.
Reviewable status: 2 of 12 files reviewed, 38 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @arai-fortanix, @dimakuv, and @kailun-qin)
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Still wrong:
- Should be
[LibOS]- The title is too long and doesn't start with a verb
I would suggest like this:
[LibOS] Emulate in/out instructions as if they generate SIGSEGV Executing I/O instructions (e.g., in/out) inside an SGX enclave generates a #UD fault. Gramine's PAL tries to handle this exception and propogates it to LibOS/app as a SIGILL signal. However, I/O instructions result in a #GP fault (which raises a SIGSEGV signal) if I/O is not permitted. Let Gramine emulate these instructions as if they end up in SIGSEGV. This helps some apps, e.g. `lscpu`.
Changed the latest commit's commit message to this.
libos/src/bookkeep/libos_signal.c line 563 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
That's too much code, it could be written in a nicer way:
siginfo_t info = { .si_signo = SIGILL, .si_code = ILL_ILLOPC, .si_addr = (void*)addr, }; if (maybe_raise_sigsegv()) { info.si_signo = SIGSEGV; info.si_code = SEGV_MAPERR; log_debug("Illegal instruction during app execution at %p, emulated as if throwing SIGSEGV;" " delivering to app", rip); } else { log_debug("Illegal instruction during app execution at %p; delivering to app", rip); }
Done.
libos/src/bookkeep/libos_signal.c line 566 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
I don't like this change to the log message. Please see my proposal in other comment.
Done.
libos/test/fs/lscpu-test.c line 26 at r1 (raw file):
Previously, kailun-qin (Kailun Qin) wrote…
I'm not a fan
I'm fine w/ using it in tests.
Done.
There was a problem hiding this comment.
Reviewed 10 of 10 files at r5, all commit messages.
Reviewable status: all files reviewed, 34 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @arai-fortanix, @kailun-qin, and @NirjharRoyiitk)
-- commits line 27 at r5:
Note to myself: don't forget to use this message when doing final rebase.
-- commits line 30 at r5:
typo: propagates (can be fixed during final rebase, no need to change now)
libos/src/bookkeep/libos_signal.c line 462 at r4 (raw file):
Previously, NirjharRoyiitk wrote…
Done.
I would make it even more specific: is_x86_instr_legacy_prefix()
libos/src/bookkeep/libos_signal.c line 465 at r4 (raw file):
Previously, NirjharRoyiitk wrote…
I kept this list of prefixes to be generic. In case it is required to check it for some other set of instructions and not specifically for IN/OUT family of instructions.
Fair enough.
libos/src/bookkeep/libos_signal.c line 476 at r4 (raw file):
Previously, NirjharRoyiitk wrote…
I kept this list of prefixes to be generic. In case it is required to check it for some other set of instructions and not specifically for IN/OUT family of instructions.
Ok, I don't mind keeping the whole list of prefixes. Indeed, maybe we'll use this helper function in other places.
But I just realized two things:
- This generic func can be used by both LibOS code and PAL code, so it should be put under
common/. - This is x86-64 specific. So it should be put under
arch/x86_64/.
Given the above, I kindly ask to move this function to a new file common/src/arch/x86_64/cpu.c (you'll need to add this new file cpu.c to corresponding meson.build). And put the definition of this func into common/include/arch/x86_64/cpu.h, so it becomes the part of the "common" API.
libos/src/bookkeep/libos_signal.c line 484 at r4 (raw file):
Previously, NirjharRoyiitk wrote…
I kept this list of prefixes to be generic. In case it is required to check it for some other set of instructions and not specifically for IN/OUT family of instructions.
OK
libos/src/bookkeep/libos_signal.c line 486 at r4 (raw file):
Previously, NirjharRoyiitk wrote…
Done.
Not done? You still have num_prefixes.
libos/src/bookkeep/libos_signal.c line 500 at r4 (raw file):
Previously, NirjharRoyiitk wrote…
Done.
I agree with Kailun, but now this list is ridiculously long. Could we make it more succinct? Like this:
0x6c, 0x6d, /* INS opcodes */
0x6e, 0x6f, /* OUTS opcodes */
...
libos/src/bookkeep/libos_signal.c line 464 at r5 (raw file):
static bool is_legacy_prefix(uint8_t op) { uint8_t prefix_list[] = { /* Group 0 */
Please remove all trailing whitespaces! Here you have a trailing space.
There are editors and tools that do this. Please run a command/tool to remove such trailing whitespace right before you push a new change. These whitespaces are very annoying.
libos/src/bookkeep/libos_signal.c line 514 at r5 (raw file):
0xef, }; size_t num_opcodes = ARRAY_SIZE(opcodes);
Please remove num_opcodes and just directly use ARRAY_SIZE
libos/src/bookkeep/libos_signal.c line 516 at r5 (raw file):
size_t num_opcodes = ARRAY_SIZE(opcodes); uint8_t* rip = (uint8_t*)context->rip; size_t num_prefixes_to_check = 4;
Why do you need this helper var? Just use 4 directly, and you may put a comment that on x86-64, there is at most 4 prefixes before the instruction opcode.
libos/src/bookkeep/libos_signal.c line 517 at r5 (raw file):
uint8_t* rip = (uint8_t*)context->rip; size_t num_prefixes_to_check = 4; /* num_prefixes_found will store the actual opcode index in rip */
What? I can't parse this comment. I would just remove it, since the code is pretty simple.
libos/src/bookkeep/libos_signal.c line 524 at r5 (raw file):
if (!is_prefix) break; }
Why this weirdly complicated logic? A simple while loop would suffice:
size_t idx = 0;
while (is_legacy_prefix(rip[idx]))
idx++;
for (size_t i = 0; i < ARRAY_SIZE(opcodes); i++)
if (rip[idx] == opcodes[i])
return true;
return false;
libos/src/bookkeep/libos_signal.c line 536 at r5 (raw file):
return is_in_out(context); }
Please remove empty line. We never use double-line delimiters.
libos/src/bookkeep/libos_signal.c line 569 at r5 (raw file):
* signal) if I/O is not permitted. Let Gramine emulate these instructions as * if they end up in SIGSEGV. This helps some apps, e.g. `lscpu`. */
You misunderstood my other comment. Please move this whole comment inside maybe_raise_sigsegv() function.
Think about it -- in this context, we don't know whether the instruction is IN/OUT or something else. All we know is that the instruction/GPR context for some reason forces us to raise a SIGSEGV instead of SIGILL. So we can't have a comment that talks about IN/OUT instructions here.
libos/src/bookkeep/libos_signal.c line 572 at r5 (raw file):
info.si_signo = SIGSEGV; info.si_code = SEGV_MAPERR; log_debug("Illegal instruction during app execution at %p, emulated as if throwing SIGSEGV;"
Please rewrap this to 100 chars per line (this is our limit in Gramine)
libos/test/regression/in_out_instruction_test.c line 1 at r5 (raw file):
/* Test Description: This test verifies that in and out instructions
Why Capital Letters?
libos/test/regression/in_out_instruction_test.c line 4 at r5 (raw file):
* correctly generate SIGSEGV. This raises SIGSEGV once for in and once for out * and then counts if number of SIGSEGVs are 2. */
This is not correctly wrapped in 100 chars per line.
libos/test/regression/in_out_instruction_test.c line 13 at r5 (raw file):
#include <stdlib.h> #include <signal.h> #include "common.h"
Please add a newline right-before this include, to make these classes of headers visually separated
libos/test/regression/in_out_instruction_test.c line 23 at r5 (raw file):
g_sigsegv_triggered++; siglongjmp(g_point, 1); return;
What's the point of this return? Please remove.
libos/test/regression/in_out_instruction_test.c line 29 at r5 (raw file):
struct sigaction int_handler = {.sa_handler=fault_handler, .sa_flags = SA_RESTART}; sigaction(SIGSEGV, &int_handler, 0);
Please add error checking everywhere. Using CHECK() for Glibc funcs, and if (...) for everything else.
libos/test/regression/in_out_instruction_test.c line 30 at r5 (raw file):
.sa_flags = SA_RESTART}; sigaction(SIGSEGV, &int_handler, 0); if (CHECK(sigsetjmp(g_point, 1)) == 0) {
Hm, I don't think sigsetjmp() returns anything except 0 and 1? So the CHECK() macro is useless.
Please double-check my thinking and remove CHECK if the function never returns errors. Ditto everywhere else in this file.
libos/test/regression/in_out_instruction_test.c line 33 at r5 (raw file):
__asm__("mov $0, %al;"); __asm__("mov $0, %dx;"); __asm__("in %dx, %al;");/* AT & T style */
This is getting annoying... Please educate yourself how inline asm works, and what input/output/clobber reg lists are.
Hint: you should use a single __asm__ and there is no need to use explicit mov instructions -- just use input regs list with assignments.
libos/test/regression/in_out_instruction_test.c line 35 at r5 (raw file):
__asm__("in %dx, %al;");/* AT & T style */ } puts("handled in instruction\n");
puts() already adds the newline symbol. You don't need \n
libos/test/regression/in_out_instruction_test.c line 35 at r5 (raw file):
__asm__("in %dx, %al;");/* AT & T style */ } puts("handled in instruction\n");
in -> IN
libos/test/regression/in_out_instruction_test.c line 41 at r5 (raw file):
__asm__("out %al, %dx;");/* AT & T style */ } puts("handled out instruction\n");
out -> OUT
libos/test/regression/in_out_instruction_test.c line 42 at r5 (raw file):
} puts("handled out instruction\n"); assert(g_sigsegv_triggered == EXPECTED_NUM_SIGSEGVS);
This can't be an assert. Asserts are removed in production builds, so your test will "ignore" the error if Gramine is built in production mode and this test is executed.
It should be a proper if {...} fail
libos/test/regression/in_out_instruction_test.c line 45 at r5 (raw file):
puts("SIGSEGV TEST OK"); return 0; }
Please add a newline symbol.
libos/test/regression/test_libos.py line 79 at r5 (raw file):
def test_003_copy_in_out_ins_test(self): stdout, stderr = self.run_binary(['in_out_instruction_test']) self.assertIn('SIGSEGV TEST OK', stdout)
The "Bootstrap" class of tests doesn't make sense for you test.
I suggest to add your test similarly to avx (very last test). But without the unittest.skipUnless(HAS_AVX, ... obviously.
dimakuv
changed the title
There was a problem hiding this comment.
Reviewable status: all files reviewed, 35 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @arai-fortanix, @kailun-qin, and @NirjharRoyiitk)
libos/test/regression/in_out_instruction_test.c line 9 at r5 (raw file):
#include <assert.h> #include <setjmp.h>
Ok, I educated myself about sigsetjmp and siglongjmp. Bar my comments, I think the implementation is correct (given that this test is single-threaded).
libos/test/regression/in_out_instruction_test.c line 17 at r5 (raw file):
#define EXPECTED_NUM_SIGSEGVS 2 static int g_sigsegv_triggered = 0; static jmp_buf g_point;
You're supposed to use sigjmp_buf instead of jmp_buf
libos/test/regression/in_out_instruction_test.c line 30 at r5 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Hm, I don't think
sigsetjmp()returns anything except 0 and 1? So theCHECK()macro is useless.Please double-check my thinking and remove CHECK if the function never returns errors. Ditto everywhere else in this file.
Double-checked myself: yes, in your program, sigsetjmp() can only return 0 or 1. So CHECK() is useless and can be removed.
NirjharRoyiitk
force-pushed
the
NirjharRoyiitk/issue-1191-in-out-instruction-throwing-SIGILL
branch
2 times, most recently
from
394fb63
to
a8e3be0
Compare
There was a problem hiding this comment.
Reviewable status: 9 of 15 files reviewed, 35 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @arai-fortanix, @dimakuv, and @kailun-qin)
libos/src/bookkeep/libos_signal.c line 462 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
I would make it even more specific:
is_x86_instr_legacy_prefix()
Done.
libos/src/bookkeep/libos_signal.c line 476 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Ok, I don't mind keeping the whole list of prefixes. Indeed, maybe we'll use this helper function in other places.
But I just realized two things:
- This generic func can be used by both LibOS code and PAL code, so it should be put under
common/.- This is x86-64 specific. So it should be put under
arch/x86_64/.Given the above, I kindly ask to move this function to a new file
common/src/arch/x86_64/cpu.c(you'll need to add this new filecpu.cto correspondingmeson.build). And put the definition of this func intocommon/include/arch/x86_64/cpu.h, so it becomes the part of the "common" API.
Done.
libos/src/bookkeep/libos_signal.c line 486 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Not done? You still have
num_prefixes.
Done.
libos/src/bookkeep/libos_signal.c line 500 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
I agree with Kailun, but now this list is ridiculously long. Could we make it more succinct? Like this:
0x6c, 0x6d, /* INS opcodes */ 0x6e, 0x6f, /* OUTS opcodes */ ...
Done.
libos/src/bookkeep/libos_signal.c line 464 at r5 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Please remove all trailing whitespaces! Here you have a trailing space.
There are editors and tools that do this. Please run a command/tool to remove such trailing whitespace right before you push a new change. These whitespaces are very annoying.
I used some settings in my editor to remove white spaces. Is it fine now?
libos/src/bookkeep/libos_signal.c line 514 at r5 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Please remove
num_opcodesand just directly useARRAY_SIZE
Done.
libos/src/bookkeep/libos_signal.c line 516 at r5 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Why do you need this helper var? Just use
4directly, and you may put a comment that on x86-64, there is at most 4 prefixes before the instruction opcode.
Done.
libos/src/bookkeep/libos_signal.c line 524 at r5 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Why this weirdly complicated logic? A simple while loop would suffice:
size_t idx = 0; while (is_legacy_prefix(rip[idx])) idx++; for (size_t i = 0; i < ARRAY_SIZE(opcodes); i++) if (rip[idx] == opcodes[i]) return true; return false;
Done.
libos/src/bookkeep/libos_signal.c line 536 at r5 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Please remove empty line. We never use double-line delimiters.
Done.
libos/src/bookkeep/libos_signal.c line 569 at r5 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
You misunderstood my other comment. Please move this whole comment inside
maybe_raise_sigsegv()function.Think about it -- in this context, we don't know whether the instruction is IN/OUT or something else. All we know is that the instruction/GPR context for some reason forces us to raise a SIGSEGV instead of SIGILL. So we can't have a comment that talks about IN/OUT instructions here.
Done.
libos/src/bookkeep/libos_signal.c line 572 at r5 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Please rewrap this to 100 chars per line (this is our limit in Gramine)
Done.
libos/test/regression/in_out_instruction_test.c line 1 at r5 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Why Capital Letters?
Done.
libos/test/regression/in_out_instruction_test.c line 4 at r5 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
This is not correctly wrapped in 100 chars per line.
total 78 characters now
Code quote:
* correctly generate SIGSEGV. This raises SIGSEGV once for IN and once for OUTlibos/test/regression/in_out_instruction_test.c line 13 at r5 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Please add a newline right-before this include, to make these classes of headers visually separated
Done.
libos/test/regression/in_out_instruction_test.c line 17 at r5 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
You're supposed to use
sigjmp_bufinstead ofjmp_buf
Done.
libos/test/regression/in_out_instruction_test.c line 23 at r5 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
What's the point of this
return? Please remove.
Done.
libos/test/regression/in_out_instruction_test.c line 29 at r5 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Please add error checking everywhere. Using
CHECK()for Glibc funcs, andif (...)for everything else.
Done.
libos/test/regression/in_out_instruction_test.c line 30 at r5 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Double-checked myself: yes, in your program,
sigsetjmp()can only return 0 or 1. SoCHECK()is useless and can be removed.
Done.
libos/test/regression/in_out_instruction_test.c line 33 at r5 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
This is getting annoying... Please educate yourself how inline asm works, and what input/output/clobber reg lists are.
Hint: you should use a single
__asm__and there is no need to use explicitmovinstructions -- just use input regs list with assignments.
Done.
libos/test/regression/in_out_instruction_test.c line 35 at r5 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
puts()already adds the newline symbol. You don't need\n
Done.
libos/test/regression/in_out_instruction_test.c line 35 at r5 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
in->IN
Done.
libos/test/regression/in_out_instruction_test.c line 41 at r5 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
out->OUT
Done.
libos/test/regression/in_out_instruction_test.c line 42 at r5 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
This can't be an assert. Asserts are removed in production builds, so your test will "ignore" the error if Gramine is built in production mode and this test is executed.
It should be a proper
if {...} fail
Done. Please let me know if the failure statement looks fine.
libos/test/regression/in_out_instruction_test.c line 45 at r5 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Please add a newline symbol.
Done.
libos/test/regression/test_libos.py line 79 at r5 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
The "Bootstrap" class of tests doesn't make sense for you test.
I suggest to add your test similarly to
avx(very last test). But without theunittest.skipUnless(HAS_AVX, ...obviously.
Done.
libos/test/fs/in_out_instruction_test.c line 30 at r4 (raw file):
Previously, kailun-qin (Kailun Qin) wrote…
NULL
not sure what this means
There was a problem hiding this comment.
Reviewed 6 of 6 files at r6, all commit messages.
Reviewable status: all files reviewed, 23 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @arai-fortanix, @kailun-qin, and @NirjharRoyiitk)
common/src/arch/x86_64/cpu.c line 2 at r6 (raw file):
/* SPDX-License-Identifier: LGPL-3.0-or-later */ /* Copyright (C) 2022 Fortnanix Inc
Typo in Fortanix
common/src/arch/x86_64/cpu.c line 8 at r6 (raw file):
/* This file contains functions that check various features and flags * specific to x86 */
We have a limit of 100 chars per line, so your sentence can fit into one line:
/* This file contains functions that check various features and flags specific to x86. */
common/src/arch/x86_64/cpu.c line 35 at r6 (raw file):
0x67, /* Address-size override prefix */ /* The rest of the prefixes aren't really applicable * for the instruction(s) we are checking.
Wait, this is now wrong. This function is generic, so it must contain all the prefixes. Please add the rest of the prefixes here.
libos/src/bookkeep/libos_signal.c line 464 at r5 (raw file):
Previously, NirjharRoyiitk wrote…
I used some settings in my editor to remove white spaces. Is it fine now?
Yes, everything looks good now :)
libos/src/bookkeep/libos_signal.c line 489 at r6 (raw file):
* signal) if I/O is not permitted. Let Gramine emulate these instructions * as if they end up in SIGSEGV. This helps some apps, e.g. `lscpu`. */
This looks like it is 80-char-per-line limited. Please fix to 100-char-per-line (please also change your text editor's settings to use 100 limit by default).
libos/test/regression/in_out_instruction_test.c line 4 at r5 (raw file):
Previously, NirjharRoyiitk wrote…
total 78 characters now
Still not done.
This looks like it is 80-char-per-line limited. Please fix to 100-char-per-line (please also change your text editor's settings to use 100 limit by default).
libos/test/regression/in_out_instruction_test.c line 29 at r5 (raw file):
Previously, NirjharRoyiitk wrote…
Done.
Not done? What about this line with sigaction(...)?
For an example, check this:
libos/test/regression/in_out_instruction_test.c line 42 at r5 (raw file):
Previously, NirjharRoyiitk wrote…
Done. Please let me know if the failure statement looks fine.
No, you should fail, not just print a message and return zero (success). Like this:
if (g_sigsegv_triggered != EXPECTED_NUM_SIGSEGVS)
errx(1, "Expected %d number of SIGSEGVs, but got only %d", EXPECTED_NUM_SIGSEGVS, g_sigsegv_triggered);
puts("SIGSEGV TEST OK");
return 0;
libos/test/regression/in_out_instruction_test.c line 30 at r6 (raw file):
.sa_flags = SA_RESTART}; int value = 0; int port = 0;
Why not pretend that this port is something meaningful, like 0x3f8 (first serial port)?
libos/test/regression/in_out_instruction_test.c line 30 at r6 (raw file):
.sa_flags = SA_RESTART}; int value = 0; int port = 0;
Hm, this can't be true? int is a 4-byte type, but the port number should be unsigned short (2-byte type)?
Also, the value is 4-byte-sized, but I would prefer a byte-sized type (unsigned char).
libos/test/regression/in_out_instruction_test.c line 33 at r6 (raw file):
sigaction(SIGSEGV, &int_handler, 0); if (sigsetjmp(g_point, 1) == 0) { __asm__ volatile("in %1, %0" : "=a"(value) : "d"(port));
I think it's better to use inb (the 1-byte-sized variant of the instruction), for clarity
libos/test/regression/in_out_instruction_test.c line 37 at r6 (raw file):
puts("handled IN instruction"); if (sigsetjmp(g_point, 1) == 0) { port = 0;
What's the point of resetting the port number? This line looks useless.
libos/test/regression/in_out_instruction_test.c line 38 at r6 (raw file):
if (sigsetjmp(g_point, 1) == 0) { port = 0; __asm__ volatile("out %0, %1" : "=a"(value) : "d"(port));
I think it's better to use outb (the 1-byte-sized variant of the instruction), for clarity
libos/test/regression/in_out_instruction_test.c line 38 at r6 (raw file):
if (sigsetjmp(g_point, 1) == 0) { port = 0; __asm__ volatile("out %0, %1" : "=a"(value) : "d"(port));
This makes no sense. =a means that the RAX register will contain the output, but the out instruction only gets the input, not the output.
Please educate yourself how in and out instruction work, and how to write inline asm for them. For example, check this: https://stackoverflow.com/questions/52355247/c-inline-asm-for-x86-in-out-port-i-o-has-operand-size-mismatch
… occurs when in/out instructions are executed. This fixes issue gramineproject#1191. This fixes the bug where in/out instructions are incorrectly generating SIGSEGV instead of SIGILL. To test this run cd libos/test/fs/ && gramine-test --sgx pytest -v -k test_211_copy_lscpu_test Signed-off-by: Nirjhar Roy <nirjhar.roy@fortanix.com>
…PF that occurs when in/out instructions are executed. Signed-off-by: Nirjhar Roy <nirjhar.roy@fortanix.com>
…PF that occurs when in/out instructions are executed. Signed-off-by: Nirjhar Roy <nirjhar.roy@fortanix.com>
…PF that occurs when in/out instructions are executed. Signed-off-by: Nirjhar Roy <nirjhar.roy@fortanix.com>
…PF that occurs when in/out instructions are executed. Signed-off-by: Nirjhar Roy <nirjhar.roy@fortanix.com>
Executing I/O instructions (e.g., in/out) inside an SGX enclave generates a #UD fault. Gramine's PAL tries to handle this exception and propogates it to LibOS/app as a SIGILL signal. However, I/O instructions result in a #GP fault (which raises a SIGSEGV signal) if I/O is not permitted. Let Gramine emulate these instructions as if they end up in SIGSEGV. This helps some apps, e.g. `lscpu`. Signed-off-by: Nirjhar Roy <nirjhar.roy@fortanix.com>
Signed-off-by: Nirjhar Roy <nirjhar.roy@fortanix.com>
NirjharRoyiitk
force-pushed
the
NirjharRoyiitk/issue-1191-in-out-instruction-throwing-SIGILL
branch
from
d13f7f8
to
fffa97f
Compare
There was a problem hiding this comment.
Reviewable status: 4 of 15 files reviewed, 22 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), period found at the end of a one-liner, "fixup! " found in commit messages' one-liners (waiting on @arai-fortanix, @dimakuv, and @kailun-qin)
common/src/arch/x86_64/cpu.c line 8 at r6 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
We have a limit of 100 chars per line, so your sentence can fit into one line:
/* This file contains functions that check various features and flags specific to x86. */
Done.
common/src/arch/x86_64/cpu.c line 35 at r6 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Wait, this is now wrong. This function is generic, so it must contain all the prefixes. Please add the rest of the prefixes here.
This function is named is_x86_instr_legacy_prefix , so shouldn't we have only the legacy prefixes? Yes this comment should be removed.
libos/src/bookkeep/libos_signal.c line 489 at r6 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
This looks like it is 80-char-per-line limited. Please fix to 100-char-per-line (please also change your text editor's settings to use 100 limit by default).
Done.
libos/test/regression/in_out_instruction_test.c line 4 at r5 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Still not done.
This looks like it is 80-char-per-line limited. Please fix to 100-char-per-line (please also change your text editor's settings to use 100 limit by default).
Done.
libos/test/regression/in_out_instruction_test.c line 42 at r5 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
No, you should fail, not just print a message and return zero (success). Like this:
if (g_sigsegv_triggered != EXPECTED_NUM_SIGSEGVS) errx(1, "Expected %d number of SIGSEGVs, but got only %d", EXPECTED_NUM_SIGSEGVS, g_sigsegv_triggered); puts("SIGSEGV TEST OK"); return 0;
Done.
libos/test/regression/in_out_instruction_test.c line 30 at r6 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Why not pretend that this port is something meaningful, like
0x3f8(first serial port)?
Done.
libos/test/regression/in_out_instruction_test.c line 30 at r6 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Hm, this can't be true?
intis a 4-byte type, but the port number should beunsigned short(2-byte type)?Also, the
valueis 4-byte-sized, but I would prefer a byte-sized type (unsigned char).
Done.
libos/test/regression/in_out_instruction_test.c line 33 at r6 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
I think it's better to use
inb(the 1-byte-sized variant of the instruction), for clarity
Done.
libos/test/regression/in_out_instruction_test.c line 37 at r6 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
What's the point of resetting the port number? This line looks useless.
Done.
libos/test/regression/in_out_instruction_test.c line 38 at r6 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
This makes no sense.
=ameans that the RAX register will contain the output, but theoutinstruction only gets the input, not the output.Please educate yourself how
inandoutinstruction work, and how to write inline asm for them. For example, check this: https://stackoverflow.com/questions/52355247/c-inline-asm-for-x86-in-out-port-i-o-has-operand-size-mismatch
Done.
There was a problem hiding this comment.
Reviewable status: 4 of 15 files reviewed, 22 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), period found at the end of a one-liner, "fixup! " found in commit messages' one-liners (waiting on @arai-fortanix, @dimakuv, and @kailun-qin)
libos/test/regression/in_out_instruction_test.c line 29 at r5 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Not done? What about this line with
sigaction(...)?For an example, check this:
Done.
libos/test/regression/in_out_instruction_test.c line 38 at r6 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
I think it's better to use
outb(the 1-byte-sized variant of the instruction), for clarity
Done.
There was a problem hiding this comment.
Reviewed 3 of 3 files at r11, all commit messages.
Reviewable status: all files reviewed, 12 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @arai-fortanix, @kailun-qin, @NirjharRoyiitk, and @oshogbo)
common/src/arch/x86_64/cpu.c line 14 at r8 (raw file):
Previously, NirjharRoyiitk wrote…
Done.
Ok, thanks for explanation. Agreed, this looks better now.
libos/test/regression/in_out_instruction_test.c line 19 at r9 (raw file):
Previously, NirjharRoyiitk wrote…
Done.
Thanks, looks good to me!
libos/test/regression/in_out_instruction_test.c line 31 at r9 (raw file):
Previously, NirjharRoyiitk wrote…
Done.
@oshogbo You're still blocking on this comment.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 12 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @arai-fortanix, @kailun-qin, and @oshogbo)
common/src/arch/x86_64/cpu.c line 14 at r8 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Ok, thanks for explanation. Agreed, this looks better now.
Done.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 5 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @arai-fortanix and @NirjharRoyiitk)
libos/src/bookkeep/libos_signal.c line 473 at r9 (raw file):
Previously, NirjharRoyiitk wrote…
Done.
I'm not sure of style of this comment: @dimakuv ?
But not blocking.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 5 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @arai-fortanix and @NirjharRoyiitk)
libos/src/bookkeep/libos_signal.c line 473 at r9 (raw file):
Previously, oshogbo (Mariusz Zaborski) wrote…
I'm not sure of style of this comment: @dimakuv ?
But not blocking.
Yep, I guess this comment does not correspond to our style. I would prefer this:
/* note that x86-64 instructions can have up to four legacy prefixes */
size_t idx = 0;
while (is_x86_instr_legacy_prefix(rip[idx]) && idx < 4)
idx++;
There was a problem hiding this comment.
Reviewable status: 14 of 15 files reviewed, 5 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @arai-fortanix and @dimakuv)
libos/src/bookkeep/libos_signal.c line 473 at r9 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Yep, I guess this comment does not correspond to our style. I would prefer this:
/* note that x86-64 instructions can have up to four legacy prefixes */ size_t idx = 0; while (is_x86_instr_legacy_prefix(rip[idx]) && idx < 4) idx++;
done
There was a problem hiding this comment.
Reviewed 1 of 1 files at r12, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @arai-fortanix)
There was a problem hiding this comment.
Dismissed @arai-fortanix from 3 discussions.
Reviewable status: all files reviewed, 2 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @NirjharRoyiitk)
There was a problem hiding this comment.
Reviewed 2 of 7 files at r2, 4 of 11 files at r7, 4 of 6 files at r10.
Reviewable status: all files reviewed, 4 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @NirjharRoyiitk)
libos/src/bookkeep/libos_signal.c line 527 at r12 (raw file):
.si_addr = (void*)addr, }; if (maybe_raise_sigsegv(context)) {
This name is misleading, it says that it may "throw a sigsegv or not", but in fact it doesn't make any changes to Gramine state (especially it can't send an exception to the app) and just returns a boolean.
This is even doubly-confusing, because 7 lines above you have maybe_emulate_syscall(context), which has the same naming convention as you used here, but it actually triggers the emulation.
Code quote:
maybe_raise_sigsegv(context)libos/test/regression/test_libos.py line 547 at r12 (raw file):
os.remove('nonexisting_testfile')
Accidental change?
There was a problem hiding this comment.
Reviewable status: all files reviewed, 4 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @mkow and @NirjharRoyiitk)
libos/src/bookkeep/libos_signal.c line 527 at r12 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
This name is misleading, it says that it may "throw a sigsegv or not", but in fact it doesn't make any changes to Gramine state (especially it can't send an exception to the app) and just returns a boolean.
This is even doubly-confusing, because 7 lines above you have
maybe_emulate_syscall(context), which has the same naming convention as you used here, but it actually triggers the emulation.
Then maybe rename this function to must_raise_sigsegv()?
There was a problem hiding this comment.
Reviewed all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @dimakuv and @NirjharRoyiitk)
libos/src/bookkeep/libos_signal.c line 527 at r12 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Then maybe rename this function to
must_raise_sigsegv()?
I don't like it, I'd rather say directly what it does: is_at_io_insn().
There was a problem hiding this comment.
Reviewable status: all files reviewed, 4 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @mkow and @NirjharRoyiitk)
libos/src/bookkeep/libos_signal.c line 527 at r12 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
I don't like it, I'd rather say directly what it does:
is_at_io_insn().
@mkow So you suggest to remove one level of indirection? (The maybe_raise_sigsegv() function which is currently a wrapper around is_in_out().)
I liked this indirection because we could have more instructions that we inject SIGSEGV into the application, and then handling of these additional instructions would go into this wrapper func.
But if you think that this wrapper func is an overkill, then I wouldn't mind to remove it.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 4 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @dimakuv and @NirjharRoyiitk)
libos/src/bookkeep/libos_signal.c line 527 at r12 (raw file):
I just wanted to rename maybe_raise_sigsegv() which currently has a name which suggests that it does something that it doesn't do. But yes, effectively it's just removing this function.
It's not a real indirection, it doesn't have any logic inside and the only thing it does is that it renames is_in_out() to something confusing.
I liked this indirection because we could have more instructions that we inject SIGSEGV into the application, and then handling of these additional instructions would go into this wrapper func.
Are there any other instructions which behave like that? This is just some weird corner case of SGX, I don't see any gains from adding more indirections to its handling.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 4 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @mkow)
libos/src/bookkeep/libos_signal.c line 527 at r12 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
I just wanted to rename
maybe_raise_sigsegv()which currently has a name which suggests that it does something that it doesn't do. But yes, effectively it's just removing this function.
It's not a real indirection, it doesn't have any logic inside and the only thing it does is that it renamesis_in_out()to something confusing.I liked this indirection because we could have more instructions that we inject SIGSEGV into the application, and then handling of these additional instructions would go into this wrapper func.
Are there any other instructions which behave like that? This is just some weird corner case of SGX, I don't see any gains from adding more indirections to its handling.
Okay I will remove this one level of indirection
There was a problem hiding this comment.
Reviewable status: all files reviewed, 4 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @mkow)
libos/test/regression/test_libos.py line 547 at r12 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
Accidental change?
Umm, which change? Sorry I am unable to view any unwanted change in git diff? Can you please point it out for me?
There was a problem hiding this comment.
Reviewable status: all files reviewed, 4 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @mkow)
libos/test/regression/test_libos.py line 547 at r12 (raw file):
Previously, NirjharRoyiitk wrote…
Umm, which change? Sorry I am unable to view any unwanted change in git diff? Can you please point it out for me?
You removed one empty line here. You can drag the mouse in the "revision history" of this file from left (the "base" symbol) to right ("r12") -- this way you'll see all the changes that your PR introduced in this particular file, and you'll see this "removal of the empty line".
There was a problem hiding this comment.
Reviewable status: 13 of 15 files reviewed, 4 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @dimakuv and @mkow)
libos/src/bookkeep/libos_signal.c line 527 at r12 (raw file):
Previously, NirjharRoyiitk wrote…
Okay I will remove this one level of indirection
done
libos/test/regression/test_libos.py line 547 at r12 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
You removed one empty line here. You can drag the mouse in the "revision history" of this file from left (the "base" symbol) to right ("r12") -- this way you'll see all the changes that your PR introduced in this particular file, and you'll see this "removal of the empty line".
Done.
There was a problem hiding this comment.
Reviewed 2 of 2 files at r13, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel) (waiting on @mkow)
There was a problem hiding this comment.
Reviewed 2 of 6 files at r6, 1 of 2 files at r13, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel) (waiting on @NirjharRoyiitk)
libos/src/bookkeep/libos_signal.c line 462 at r13 (raw file):
} static bool is_in_out(PAL_CONTEXT* context) {
This is x86-64 specific, should be in arch/x86_64/, same as is_x86_instr_legacy_prefix(), which you already placed there.
libos/src/bookkeep/libos_signal.c line 518 at r13 (raw file):
}; if (is_in_out(context)) { /* Executing I/O instructions (e.g., in/out) inside an SGX enclave generates a #UD fault.
Actually, this whole logic is x86-64+SGX-specific, shouldn't it go to Linux-SGX PAL, into the arch/x86_64 dir?
This file should contain only architecture-agnostic and backend-agnostic code, otherwise people like @stefanberger will have problems with their ports to other architectures.
See #1166 for a reference how to implement such a change in a portable way.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 4 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel) (waiting on @mkow and @NirjharRoyiitk)
libos/src/bookkeep/libos_signal.c line 518 at r13 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
Actually, this whole logic is x86-64+SGX-specific, shouldn't it go to Linux-SGX PAL, into the
arch/x86_64dir?
This file should contain only architecture-agnostic and backend-agnostic code, otherwise people like @stefanberger will have problems with their ports to other architectures.See #1166 for a reference how to implement such a change in a portable way.
Indeed, it would be nice if is_in_out() could become arch_is_in_out() and be handled in an x86-specific source file. On other archs this function would then become just an inline returning false.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 4 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel) (waiting on @NirjharRoyiitk and @stefanberger)
libos/src/bookkeep/libos_signal.c line 518 at r13 (raw file):
Previously, stefanberger (Stefan Berger) wrote…
Indeed, it would be nice if is_in_out() could become arch_is_in_out() and be handled in an x86-specific source file. On other archs this function would then become just an inline returning false.
IMO no reason to even implement it on other archs, as the logic using it would be placed in Linux-SGX PAL, which is x86-only.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 4 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel) (waiting on @mkow and @NirjharRoyiitk)
libos/src/bookkeep/libos_signal.c line 518 at r13 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
IMO no reason to even implement it on other archs, as the logic using it would be placed in Linux-SGX PAL, which is x86-only.
@mkow I feel like the PAL code is not ready/suitable for such remapping (PAL would need to remap from #UD to #GP and then call a corresponding memfault_upcall() LibOS function).
What our current PAL code can do is to:
- either emulate the hardware exception inside PAL itself,
- or forward the hardware exception to LibOS (this is in case of
syscallinstruction currently).
I'm unsure whether #UD -> #GP is the job for PAL code...
There was a problem hiding this comment.
Reviewable status: all files reviewed, 4 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel) (waiting on @dimakuv, @NirjharRoyiitk, and @stefanberger)
libos/src/bookkeep/libos_signal.c line 518 at r13 (raw file):
I feel like the PAL code is not ready/suitable for such remapping
Why not? Right now PAL gets the all the exceptions, process them and then calls LibOS, passing information about what happened. What's the problem with SGX PAL passing more suitable information to LibOS, free from SGX-specific quirks?
What our current PAL code can do is to:
- either emulate the hardware exception inside PAL itself,
We can't do this, the app may want to handle it.
I'm unsure whether
#UD -> #GPis the job for PAL code...
It definitely is? PAL's role is to abstract out the specific backend (host OS, hardware quirks, etc).
There was a problem hiding this comment.
Reviewable status: all files reviewed, 4 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @mkow and @NirjharRoyiitk)
libos/src/bookkeep/libos_signal.c line 518 at r13 (raw file):
It definitely is? PAL's role is to abstract out the specific backend (host OS, hardware quirks, etc).
Ok, I agree. @NirjharRoyiitk You'll need to move your code from LibOS to Linux-SGX PAL... Unfortunately, it will be a bit more involved than what you have now in LibOS.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 5 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @mkow and @NirjharRoyiitk)
a discussion (no related file):
Quick update: we hit this problem on the latest PyTorch.
Signed-off-by: Nirjhar Roy <nirjhar.roy@fortanix.com>
There was a problem hiding this comment.
Reviewable status: 12 of 17 files reviewed, 5 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @dimakuv and @mkow)
a discussion (no related file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Quick update: we hit this problem on the latest PyTorch.
Done.
libos/src/bookkeep/libos_signal.c line 462 at r13 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
This is x86-64 specific, should be in
arch/x86_64/, same asis_x86_instr_legacy_prefix(), which you already placed there.
moved it to common/src/arch/x86_64/cpu.c
libos/src/bookkeep/libos_signal.c line 518 at r13 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
It definitely is? PAL's role is to abstract out the specific backend (host OS, hardware quirks, etc).
Ok, I agree. @NirjharRoyiitk You'll need to move your code from LibOS to Linux-SGX PAL... Unfortunately, it will be a bit more involved than what you have now in LibOS.
I am now detecting the opcode in pal_exception.c in PAL linux-sgx and sending the appropriate event/signal (SIGSEGV/mem fault)
There was a problem hiding this comment.
Reviewed 1 of 3 files at r11, 5 of 5 files at r14, all commit messages.
Reviewable status: all files reviewed, 20 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @dimakuv, @NirjharRoyiitk, and @oshogbo)
common/src/arch/x86_64/cpu.c line 27 at r14 (raw file):
0x2e, /* CS segment override */ 0x3e, /* DS segment override */
0x2e, /* Branch not taken */ 0x3e, /* Branch taken */
duplicates?
common/src/arch/x86_64/cpu.c line 62 at r14 (raw file):
x86-64 instructions can have up to four legacy prefixes
source?
common/src/arch/x86_64/meson.build line 5 at r14 (raw file):
) common_src_arch_c = files('cpu.c')
Suggestion:
common_src_arch_c = files(
'cpu.c',
)libos/src/bookkeep/libos_signal.c line 482 at r14 (raw file):
siginfo_t info = { .si_signo = SIGILL, .si_code = ILL_ILLOPC,
why unindent?
libos/src/bookkeep/libos_signal.c line 485 at r14 (raw file):
.si_addr = (void*)addr, }; log_debug("Illegal instruction during app execution at %p; delivering to app", rip);
Any reason for this change?
pal/src/host/linux-sgx/pal_exception.c line 136 at r14 (raw file):
/* return value: true if #UD was handled and execution can be continued without propagating #UD; * false if #UD was not handled and exception needs to be raised up to LibOS/app */ static bool handle_ud(sgx_cpu_context_t* uc, int *event_num_ptr) {
Suggestion:
int* out_event_numpal/src/host/linux-sgx/pal_exception.c line 178 at r14 (raw file):
/* Executing I/O instructions (e.g., in/out) inside an SGX enclave generates a #UD fault. * Gramine's PAL tries to handle this exception and propagates it to LibOS/app as a * SIGILL signal. However, I/O instructions result in a #GP fault (which raises a
PAL doesn't send SIGILL to LibOS, it uses PAL API, not Linux API.
pal/src/host/linux-sgx/pal_exception.c line 180 at r14 (raw file):
* SIGILL signal. However, I/O instructions result in a #GP fault (which raises a * SIGSEGV signal) if I/O is not permitted. Let Gramine emulate these instructions as if * they end up in SIGSEGV. This helps some apps, e.g. `lscpu`.
No need to list this here for justification, it's obvious that we want to emulate the 1-1 behavior of the normal x86 environment.
Code quote:
This helps some apps, e.g. `lscpu`.pal/src/host/linux-sgx/pal_exception.c line 183 at r14 (raw file):
*/ log_debug("Illegal instruction during app execution at %p, emulated as if " "throwing SIGSEGV; delivering to app", instr);
We shouldn't print this on every instruction emulated, printing is slow.
pal/src/host/linux-sgx/pal_exception.c line 183 at r14 (raw file):
*/ log_debug("Illegal instruction during app execution at %p, emulated as if " "throwing SIGSEGV; delivering to app", instr);
Please rewrite the warning text, it should say that we're sending SIGSEGV because an IN/OUT instruction was executed.
libos/test/regression/in_out_instruction.c line 7 at r14 (raw file):
/* Test description: this test verifies that in and out instructions correctly generate SIGSEGV. * This raises SIGSEGV once for IN and once for OUT and then counts if number of SIGSEGVs is 2.
- It's obvious that this is the test description from the first 3 words.
- This test doesn't send SIGSEGV, it executes IN/OUT and checks whether SIGSEGV arrived.
- In a single paragraph you are formatting in/out in two different ways.
Suggestion:
/* This test verifies whether `in` and `out` instructions correctly generate SIGSEGV,
* by executing `in` and `out` once and then counting if the number of SIGSEGVs received is 2.libos/test/regression/in_out_instruction.c line 11 at r14 (raw file):
#define _GNU_SOURCE #define EXPECTED_NUM_SIGSEGVS 2
This should be after includes, together with global variables (now it looks like this was some macro to change the include mode, like _GNU_SOURCE).
libos/test/regression/in_out_instruction.c line 29 at r14 (raw file):
unsigned char inb_func(unsigned short port) __attribute__((visibility("internal"))); void outb_func(unsigned char value, unsigned short port) __attribute__((visibility("internal")));
ditto for other places
Suggestion:
uint8_t value, uint16_t portlibos/test/regression/in_out_instruction.c line 69 at r14 (raw file):
} g_sigsegv_triggered++; siglongjmp(g_point, 1);
Please set RIP to ret instruction instead and remove all setjumps.
libos/test/regression/in_out_instruction.c line 90 at r14 (raw file):
if (g_sigsegv_triggered != EXPECTED_NUM_SIGSEGVS) errx(1, "Expected %d number of SIGSEGVs, but got only %d", EXPECTED_NUM_SIGSEGVS,
Suggestion:
"Expected %d SIGSEGVslibos/test/regression/in_out_instruction_test.c line 20 at r9 (raw file):
Previously, oshogbo (Mariusz Zaborski) wrote…
Ok!
I'd still change it to CHECK() as @oshogbo suggested - on Linux this would be assert(), but under Gramine this is one of the things we want to test - whether signal emulation works as expected.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 21 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @mkow, @NirjharRoyiitk, and @oshogbo)
a discussion (no related file):
@NirjharRoyiitk Will you work on this PR? Otherwise I can take over and finish it.
Description of the changes
Fixes #1191 :
inandoutinstructions causeSIGILLinstead ofSIGSEGV.inandoutinstructions aren't allowed inside SGX enclaves and hence throwsSIGILLbut native behavior of user applications is thatin/outinstructions throwSIGSEGV.lscpuis requires this fix andlscpuis sometimes used in tensorflow applications and that is why this was required.https://c9x.me/x86/html/file_module_x86_id_139.html (for IN instruction)
https://c9x.me/x86/html/file_module_x86_id_222.html (for OUT instruction)
https://c9x.me/x86/html/file_module_x86_id_139.html
https://stackoverflow.com/questions/3215878/what-are-in-out-instructions-in-x86-used-for
https://stackoverflow.com/questions/46642384/how-to-run-code-that-uses-x86-in-and-out-i-o-instructions
Fixes #1191
How to test this PR?
cd libos/test/fs/ && gramine-test --sgx pytest -v -k test_211_copy_lscpu_test--> Run this command from Gramine root repoThis change is