From 92afac2a0ca640f19d39da6e7e82e1acb93e2024 Mon Sep 17 00:00:00 2001 From: Alice Frosi Date: Wed, 10 May 2023 11:06:12 +0200 Subject: Refactoring of gluten_read/write Refactor includes: - use static inline instead of macro - return -1 if there is an error and don't exit - eval return 0 or -1 - adjust code and tests --- common/gluten.h | 60 +++++++++++++++++++++++++------------------- operations.c | 28 +++++++++++++-------- operations.h | 4 +-- tests/unit/test_errors.c | 38 +++++++++++++++++----------- tests/unit/test_operations.c | 16 ++++++------ 5 files changed, 86 insertions(+), 60 deletions(-) diff --git a/common/gluten.h b/common/gluten.h index d1d3c61..078c6fa 100644 --- a/common/gluten.h +++ b/common/gluten.h @@ -12,6 +12,8 @@ #include #include +#include + #include "util.h" extern struct seccomp_data anonymous_seccomp_data; @@ -26,30 +28,6 @@ extern struct seccomp_data anonymous_seccomp_data; MAX(MAX(MAX(DATA_SIZE, RO_DATA_SIZE), INST_MAX), \ ARRAY_SIZE(anonymous_seccomp_data.args)) -#define check_gluten_limits(g, v, size) \ - do { \ - struct gluten_offset off = { v.type, v.offset + (size) }; \ - if (!is_offset_valid(off)) \ - die(" invalid offset: %d", off.offset); \ - } while (0) - -#define gluten_write(g, dst, src) \ - do { \ - void *p = gluten_write_ptr((g), (dst)); \ - check_gluten_limits((g), (dst), sizeof((src))); \ - if (p == NULL) \ - die(" invalid type of offset"); \ - memcpy(p, &(src), sizeof(src)); \ - } while (0) - -#define gluten_read(s, g, dst, src, size) \ - do { \ - const void *p = gluten_ptr((s), (g), (src)); \ - check_gluten_limits((g), (src), (size)); \ - if (p == NULL) \ - die(" invalid type of offset"); \ - memcpy(&(dst), p, (size)); \ - } while (0) enum gluten_offset_type { OFFSET_RO_DATA = 0, @@ -242,7 +220,7 @@ static inline void *gluten_write_ptr(struct gluten *g, #endif { if (!is_offset_valid(x)) - die(" invalid offset: %d", x.offset); + return NULL; switch (x.type) { case OFFSET_DATA: @@ -264,7 +242,7 @@ static inline const void *gluten_ptr(const struct seccomp_data *s, const struct gluten_offset x) { if (!is_offset_valid(x)) - die(" invalid offset: %d", x.offset); + return NULL; switch (x.type) { case OFFSET_DATA: @@ -279,5 +257,35 @@ static inline const void *gluten_ptr(const struct seccomp_data *s, return NULL; } } + +static inline bool check_gluten_limits(struct gluten_offset v, size_t size) +{ + struct gluten_offset off = { v.type, v.offset + size }; + return is_offset_valid(off); +} + +static inline int gluten_write(struct gluten *g, struct gluten_offset dst, + const void *src, size_t size) +{ + void *p = gluten_write_ptr(g, dst); + if (p == NULL || !check_gluten_limits(dst, size)) + return -1; + memcpy(p, src, size); + + return 0; +} + +static inline int gluten_read(const struct seccomp_data *s, struct gluten *g, + void *dst, const struct gluten_offset src, + size_t size) +{ + const void *p = gluten_ptr(s, g, src); + if (p == NULL || !check_gluten_limits(src, size)) + return -1; + memcpy(dst, p, size); + + return 0; +} + #endif #endif /* COMMON_GLUTEN_H */ diff --git a/operations.c b/operations.c index bf03ab8..af86568 100644 --- a/operations.c +++ b/operations.c @@ -177,7 +177,10 @@ int op_load(const struct seccomp_notif *req, int notifier, struct gluten *g, ret = -1; goto out; } - check_gluten_limits(&g, load->dst, load->size); + if (!check_gluten_limits(load->dst, load->size)) { + ret = -1; + goto out; + } if (pread(fd, gluten_write_ptr(g, load->dst), load->size, *src) < 0) { perror("pread"); return -1; @@ -233,7 +236,7 @@ int op_call(const struct seccomp_notif *req, int notifier, struct gluten *g, * reference */ if (op->has_ret) - gluten_write(g, op->ret, c.ret); + return gluten_write(g, op->ret, &c.ret, sizeof(c.ret)); return 0; } @@ -264,7 +267,8 @@ int op_return(const struct seccomp_notif *req, int notifier, struct gluten *g, resp.flags = 0; resp.error = 0; - gluten_read(NULL, g, resp.val, op->val, sizeof(resp.val)); + if (gluten_read(&req->data, g, &resp.val, op->val, sizeof(resp.val)) == -1) + return -1; if (send_target(&resp, notifier) == -1) return -1; @@ -300,8 +304,10 @@ static int do_inject(const struct seccomp_notif *req, int notifier, resp.newfd_flags = 0; resp.id = req->id; - gluten_read(NULL, g, resp.newfd, op->new_fd, sizeof(resp.newfd)); - gluten_read(NULL, g, resp.srcfd, op->old_fd, sizeof(resp.srcfd)); + if(gluten_read(NULL, g, &resp.newfd, op->new_fd, sizeof(resp.newfd)) == -1) + return -1; + if(gluten_read(NULL, g, &resp.srcfd, op->old_fd, sizeof(resp.srcfd)) == -1) + return -1; if (atomic) resp.flags |= SECCOMP_ADDFD_FLAG_SEND; @@ -351,8 +357,10 @@ int op_resolve_fd(const struct seccomp_notif *req, int notifier, (void)notifier; - gluten_read(NULL, g, path, op->path, sizeof(op->path_size)); - gluten_read(NULL, g, fd, op->fd, sizeof(fd)); + if(gluten_read(NULL, g, &path, op->path, sizeof(op->path_size)) == -1) + return -1; + if(gluten_read(NULL, g, &fd, op->fd, sizeof(fd)) == -1) + return -1; snprintf(fdpath, PATH_MAX, "/proc/%d/fd/%d", req->pid, fd); if ((nbytes = readlink(fdpath, buf, op->path_size)) < 0) { @@ -366,12 +374,12 @@ int op_resolve_fd(const struct seccomp_notif *req, int notifier, return 0; } -void eval(struct gluten *g, struct op *ops, const struct seccomp_notif *req, +int eval(struct gluten *g, struct op *ops, const struct seccomp_notif *req, int notifier) { struct op *op = ops; - while (op->type != OP_END && op != NULL) { + while (op->type != OP_END) { switch (op->type) { HANDLE_OP(OP_CALL, op_call, call); HANDLE_OP(OP_BLOCK, op_block, block); @@ -384,7 +392,7 @@ void eval(struct gluten *g, struct op *ops, const struct seccomp_notif *req, HANDLE_OP(OP_RESOLVEDFD, op_resolve_fd, resfd); default: fprintf(stderr, "unknown operation %d \n", op->type); - return; } } + return 0; } diff --git a/operations.h b/operations.h index 011369a..ccc42c2 100644 --- a/operations.h +++ b/operations.h @@ -21,7 +21,7 @@ if (res == 0) \ (op)++; \ else if (res == -1) \ - (op) = NULL; \ + return -1; \ else \ (op) += res; \ } while (0); \ @@ -35,7 +35,7 @@ struct arg_clone { }; int do_call(struct arg_clone *c); -void eval(struct gluten *g, struct op *ops, const struct seccomp_notif *req, +int eval(struct gluten *g, struct op *ops, const struct seccomp_notif *req, int notifier); int op_call(const struct seccomp_notif *req, int notifier, struct gluten *g, struct op_call *op); diff --git a/tests/unit/test_errors.c b/tests/unit/test_errors.c index d00d42e..ca6fcb0 100644 --- a/tests/unit/test_errors.c +++ b/tests/unit/test_errors.c @@ -1,4 +1,3 @@ -#include #include #include #include @@ -31,6 +30,7 @@ START_TEST(test_bound_check) { struct op ops[] = { { OP_RETURN, { { 0 } } }, + { OP_END, { { 0 } } }, }; ops[0].op.ret.val.offset = test_max_size_data[_i].offset; ops[0].op.ret.val.type = test_max_size_data[_i].type; @@ -45,9 +45,10 @@ START_TEST(test_write_op_return) { .call = { .nr = __NR_getpid, .has_ret = true, .ret = { OFFSET_DATA, DATA_SIZE - 1 } } } }, + { OP_END, { { 0 } } }, }; - eval(&gluten, ops, &req, notifyfd); + ck_assert_int_eq(eval(&gluten, ops, &req, notifyfd), -1); } START_TEST(test_write_op_load) @@ -58,20 +59,28 @@ START_TEST(test_write_op_load) { .load = { { OFFSET_SECCOMP_DATA, 1 }, { OFFSET_DATA, DATA_SIZE - 1 }, sizeof(a) } } }, + { OP_END, { { 0 } } }, }; - eval(&gluten, ops, &req, notifyfd); + ck_assert_int_eq(eval(&gluten, ops, &req, notifyfd), -1); } +struct gluten_offset test_max_size_read_data[] = { + { OFFSET_DATA, DATA_SIZE }, + { OFFSET_RO_DATA, RO_DATA_SIZE }, + { OFFSET_SECCOMP_DATA, 6 }, +}; + START_TEST(test_read_op_return) { struct op ops[] = { { OP_RETURN, { { 0 } } }, + { OP_END, { { 0 } } }, }; - ops[0].op.ret.val.offset = test_max_size_data[_i].offset - 1; - ops[0].op.ret.val.type = test_max_size_data[_i].type; + ops[0].op.ret.val.offset = test_max_size_read_data[_i].offset - 1; + ops[0].op.ret.val.type = test_max_size_read_data[_i].type; - eval(&gluten, ops, &req, notifyfd); + ck_assert_int_eq(eval(&gluten, ops, &req, notifyfd), -1); } Suite *error_suite(void) @@ -82,21 +91,22 @@ Suite *error_suite(void) s = suite_create("Error handling"); bounds = tcase_create("bound checks"); - tcase_add_loop_exit_test(bounds, test_bound_check, EXIT_FAILURE, 0, - sizeof(test_max_size_data) / - sizeof(test_max_size_data[0])); + tcase_add_loop_test(bounds, test_bound_check, 0, + sizeof(test_max_size_data) / + sizeof(test_max_size_data[0])); suite_add_tcase(s, bounds); gwrite = tcase_create("write gluten"); tcase_add_checked_fixture(gwrite, setup_error_check, teardown); - tcase_add_exit_test(gwrite, test_write_op_return, EXIT_FAILURE); - tcase_add_exit_test(gwrite, test_write_op_load, EXIT_FAILURE); + tcase_add_test(gwrite, test_write_op_return); + tcase_add_test(gwrite, test_write_op_load); suite_add_tcase(s, gwrite); gread = tcase_create("read gluten"); - tcase_add_loop_exit_test(gread, test_read_op_return, EXIT_FAILURE, 0, - sizeof(test_max_size_data) / - sizeof(test_max_size_data[0])); + tcase_add_checked_fixture(gread, setup_error_check, teardown); + tcase_add_loop_test(gread, test_read_op_return, 0, + sizeof(test_max_size_read_data) / + sizeof(test_max_size_read_data[0])); suite_add_tcase(s, gread); return s; diff --git a/tests/unit/test_operations.c b/tests/unit/test_operations.c index 9c3cf94..0e22754 100644 --- a/tests/unit/test_operations.c +++ b/tests/unit/test_operations.c @@ -136,7 +136,7 @@ START_TEST(test_op_call) { 0 }, }; - eval(&gluten, operations, &req, notifyfd); + ck_assert_int_eq(eval(&gluten, operations, &req, notifyfd), 0); check_target_result(1, 0, true); } END_TEST @@ -153,7 +153,7 @@ START_TEST(test_op_call_ret) { 0 }, }; - eval(&gluten, operations, &req, notifyfd); + ck_assert_int_eq(eval(&gluten, operations, &req, notifyfd), 0); check_target_result(1, 0, true); ck_read_gluten(gluten, operations[0].op.call.ret, r); ck_assert(r == getpid()); @@ -218,7 +218,7 @@ START_TEST(test_op_load) int v = 2; ck_write_gluten(gluten, operations[1].op.ret.val, v); - eval(&gluten, operations, &req, notifyfd); + ck_assert_int_eq(eval(&gluten, operations, &req, notifyfd), 0); check_target_result(v, 0, false); ck_read_gluten(gluten, operations[0].op.load.dst, addr); @@ -246,7 +246,7 @@ static void test_op_cmp_int(int a, int b, enum op_cmp_type cmp) ck_write_gluten(gluten, operations[0].op.cmp.x, a); ck_write_gluten(gluten, operations[0].op.cmp.y, b); - eval(&gluten, operations, &req, notifyfd); + ck_assert_int_eq(eval(&gluten, operations, &req, notifyfd), 0); check_target_result_nonegative(); } @@ -306,7 +306,7 @@ START_TEST(test_op_cmp_string_eq) ck_write_gluten(gluten, operations[0].op.cmp.x, s1); ck_write_gluten(gluten, operations[0].op.cmp.y, s2); - eval(&gluten, operations, &req, notifyfd); + ck_assert_int_eq(eval(&gluten, operations, &req, notifyfd), 0); check_target_result_nonegative(); } END_TEST @@ -332,7 +332,7 @@ START_TEST(test_op_cmp_string_false) ck_write_gluten(gluten, operations[0].op.cmp.x, s1); ck_write_gluten(gluten, operations[0].op.cmp.y, s2); - eval(&gluten, operations, &req, notifyfd); + ck_assert_int_eq(eval(&gluten, operations, &req, notifyfd), 0); check_target_result_nonegative(); } END_TEST @@ -355,7 +355,7 @@ START_TEST(test_op_resolvedfd_eq) ck_write_gluten(gluten, operations[0].op.resfd.fd, at->fd); ck_write_gluten(gluten, operations[0].op.resfd.path, path); - eval(&gluten, operations, &req, notifyfd); + ck_assert_int_eq(eval(&gluten, operations, &req, notifyfd), 0); check_target_result(-1, 1, false); } END_TEST @@ -379,7 +379,7 @@ START_TEST(test_op_resolvedfd_neq) ck_write_gluten(gluten, operations[0].op.resfd.fd, at->fd); ck_write_gluten(gluten, operations[0].op.resfd.path, path2); - eval(&gluten, operations, &req, notifyfd); + ck_assert_int_eq(eval(&gluten, operations, &req, notifyfd), 0); check_target_result(-1, 1, false); } END_TEST -- cgit v1.2.3