Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(handle_t): redef handle_t as pointer #16

Merged
merged 2 commits into from
Aug 24, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions .github/workflows/blacklist.ptr.cex.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
storage_ipc_indirect_handlers_unsat_test
storage_ipc_msg_buffer_unsat_test
gatekeeper_ipc_unsat_test
# following tracked in https://github.com/seahorn/verifyTrusty/issues/18
storage_ipc_port_create_destroy_unsat_test
# following tracked in https://github.com/seahorn/verifyTrusty/issues/19
port_send_msg_pf_unsat_test
3 changes: 3 additions & 0 deletions .github/workflows/blacklist.ptr.vac.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
storage_ipc_indirect_handlers_unsat_test
storage_ipc_msg_buffer_unsat_test
gatekeeper_ipc_unsat_test
23 changes: 15 additions & 8 deletions .github/workflows/main.yml
Original file line number Diff line number Diff line change
Expand Up @@ -17,23 +17,30 @@ jobs:
verify_flag: ["", "--vac", "--cex"]

steps:
- name: Checkout
- name: Checkout
uses: actions/checkout@master

- name: Get type
id: type
run: echo ::set-output name=type::$(echo ${{ matrix.verify_flag }} | awk '{ print substr($1,3,5) "" substr($2,22) }')

- name: Get exclude test
id: exclude_files
run: (test -f ".github/workflows/blacklist.${{steps.type.outputs.type}}.txt" && echo ::set-output name=exclude_files::"($(cut -f 1 ".github/workflows/blacklist.${{steps.type.outputs.type}}.txt" | head -c -1 | tr '\n' '|'))" ) || (echo ::set-output name=exclude_files::" ")

- name: Docker

- name: Get exclude test (for handle-type-is-ptr)
id: exclude_files_ptr
run: (test -f ".github/workflows/blacklist.ptr.${{steps.type.outputs.type}}.txt" && echo ::set-output name=exclude_files::"($(cut -f 1 ".github/workflows/blacklist.ptr.${{steps.type.outputs.type}}.txt" | head -c -1 | tr '\n' '|'))" ) || (echo ::set-output name=exclude_files::" ")

- name: Docker
run: docker build -t verify-trusty . --file docker/seahorn-dev10-verify-trusty.Dockerfile

- name: Get number of Cores
id: cores
run: echo "::set-output name=num_cores::$(nproc --all)"
- name: Run Tests

- name: Run Tests
run: docker run -t verify-trusty /bin/bash -c "cd build && mkdir -p /tmp/verifyTrusty && env VERIFY_FLAGS=\"${{ matrix.verify_flag }}\" ctest -j ${{steps.cores.outputs.num_cores}} --output-on-failure --timeout 2000 -E \"${{ steps.exclude_files.outputs.exclude_files }}\""

- name: Run handle-type-is-ptr Tests
run: docker run -t verify-trusty /bin/bash -c "cd build_ptr && mkdir -p /tmp/verifyTrusty && env VERIFY_FLAGS=\"${{ matrix.verify_flag }}\" ctest -j ${{steps.cores.outputs.num_cores}} --output-on-failure --timeout 2000 -E \"${{ steps.exclude_files_ptr.outputs.exclude_files }}\""
19 changes: 15 additions & 4 deletions CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -29,9 +29,10 @@ set(LLVMIR_OPT ${SEA_OPT})
set(CPPSTDLIB "libc++" CACHE STRING "C++ std library to use")

set(TRUSTY_ROOT ${CMAKE_CURRENT_SOURCE_DIR}/trusty)
set(TRUSTY_MOD_ROOT ${CMAKE_CURRENT_SOURCE_DIR}/trusty_mod)
set(TRUSTY_TARGET "arm32" CACHE STRING "Trusty target for verification")
set(EXTERNAL_ROOT ${TRUSTY_ROOT}/external)

option(HANDLE_TYPE_IS_PTR "Build SHADOW MEM versions of jobs" OFF)
# set C++ stdlib
string(APPEND CMAKE_CXX_FLAGS " -stdlib=${CPPSTDLIB}")

Expand All @@ -53,8 +54,14 @@ set(VERIFY_CMD ${CMAKE_CURRENT_BINARY_DIR}/verify)
include_directories(BEFORE ${CMAKE_CURRENT_BINARY_DIR}/include)

include_directories(seahorn/trusty_common/include)
include_directories(seahorn/trusty/user/base/lib/libc-trusty)

# TODO: Don't include at global level
# I don't get why we have two directories trusty_common and trusty
# and why is trusty included as an implicit dep of trusty_common
if (HANDLE_TYPE_IS_PTR)
include_directories(seahorn/trusty/user/base/lib/libc-trusty-ptr)
else ()
include_directories(seahorn/trusty/user/base/lib/libc-trusty)
endif ()
# Include header's directories
include_directories(${TRUSTY_ROOT}/hardware/libhardware/include)
include_directories(${TRUSTY_ROOT}/trusty/user/base/interface/storage/include)
Expand All @@ -63,7 +70,11 @@ include_directories(${TRUSTY_ROOT}/trusty/user/base/interface/keymaster/include)
include_directories(${TRUSTY_ROOT}/trusty/user/base/interface/hwkey/include)
include_directories(${TRUSTY_ROOT}/trusty/user/base/include/uapi)
include_directories(${TRUSTY_ROOT}/trusty/user/base/include/shared)
include_directories(${TRUSTY_ROOT}/trusty/user/base/include/user)
if (HANDLE_TYPE_IS_PTR)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@priyasiddharth you can also add HANDLE_TYPE_IS_PTR to definitions using CMake add_definitions command and then use it as #ifdef inside header files. This might be simpler than changing include directories with cmake.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A simple solution would delegate the decision to each include point and would duplicate the conditional for evey use. A more complex solution would be to have a wrapper header which makes the decision internally. It's not clear why that's needed for now.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was suggesting a wrapper header, simply because this is the typical way to address this problem. This guarantees less unexpected issues later on.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We discussed this in https://seahornteam.slack.com/archives/DPND2KEJU/p1659124357825129.

conclusion: to use wrap header file you would need to merge common files rather than have two completely different source trees

This requires adding more moving pieces to the build and IMO is overkill for now. My vote is to submit this PR and revisit this if it needs more engineering to make it robust -- once it moves beyond being a proof of concept.

include_directories(${TRUSTY_MOD_ROOT}/trusty/user/base/include/user)
else ()
include_directories(${TRUSTY_ROOT}/trusty/user/base/include/user)
endif ()
include_directories(${TRUSTY_ROOT}/trusty/user/base/lib/libc-trusty/include)
include_directories(${TRUSTY_ROOT}/trusty/user/base/lib/libstdc++-trusty/include)
include_directories(${TRUSTY_ROOT}/trusty/user/base/lib/libcxxabi-trusty/include)
Expand Down
4 changes: 4 additions & 0 deletions docker/seahorn-dev10-verify-trusty.Dockerfile
Original file line number Diff line number Diff line change
Expand Up @@ -58,5 +58,9 @@ RUN echo "Installing Trusty" && \
WORKDIR /home/usea/verifyTrusty
RUN mkdir build && cd build && cmake -DSEA_LINK=llvm-link-10 -DCMAKE_C_COMPILER=clang-10 -DCMAKE_CXX_COMPILER=clang++-10 -DSEAHORN_ROOT=/home/usea/seahorn -DTRUSTY_TARGET=x86_64 ../ -GNinja && cmake --build .

## Also generate jobs using handle_t as ptr
WORKDIR /home/usea/verifyTrusty
RUN mkdir build_ptr && cd build_ptr && cmake -DSEA_LINK=llvm-link-10 -DCMAKE_C_COMPILER=clang-10 -DCMAKE_CXX_COMPILER=clang++-10 -DSEAHORN_ROOT=/home/usea/seahorn -DTRUSTY_TARGET=x86_64 -DHANDLE_TYPE_IS_PTR=ON ../ -GNinja && cmake --build .

## set default user and wait for someone to login and start running verification tasks
USER usea
18 changes: 13 additions & 5 deletions seahorn/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -169,20 +169,24 @@ function(sea_attach_bc name)
target_compile_options(${SOURCE_EXE} PUBLIC -O1 -Xclang -disable-llvm-optzns -S)
target_compile_options(${SOURCE_EXE} PUBLIC -Wno-macro-redefined)
target_compile_options(${SOURCE_EXE} PUBLIC -g)
target_compile_options(${SOURCE_EXE} PUBLIC -Werror) # warnings are errors
target_compile_options(${SOURCE_EXE} PUBLIC -fdeclspec)
target_compile_options(${SOURCE_EXE} PUBLIC -Wstrict-prototypes -Wwrite-strings)

#if (HANDLE_TYPE_IS_PTR)
# target_compile_options(${SOURCE_EXE} PUBLIC -m32) # use 32-bit ptr for x86_64 arch. See https://gcc.gnu.org/onlinedocs/gcc/x86-Options.html.
#endif()
# work-around for Mac
if (NOT APPLE)
target_compile_options(${SOURCE_EXE} PUBLIC -target ${TARGET_ARCH}-linux-gnu)
endif()
endif ()

if (${TRUSTY_TARGET} STREQUAL "arm32")
target_compile_options(${SOURCE_EXE} PRIVATE -mfloat-abi=softfp
-fstack-protector-strong -mthumb
)
-fstack-protector-strong -mthumb
)
target_compile_definitions(${SOURCE_EXE} PRIVATE __thumb__)
elseif(${TRUSTY_TARGET} STREQUAL "x86_64")
elseif (${TRUSTY_TARGET} STREQUAL "x86_64")
target_compile_options(${SOURCE_EXE} PRIVATE -mcmodel=kernel -mno-red-zone -msoft-float
-fno-pic -ffunction-sections -fdata-sections -fno-stack-protector -fasynchronous-unwind-tables )
else ()
Expand Down Expand Up @@ -265,5 +269,9 @@ add_subdirectory(trusty_common)
add_subdirectory(trusty)

add_subdirectory(storage)
add_subdirectory(gatekeeper)
if (HANDLE_TYPE_IS_PTR)
# do nothing for MVP
else ()
add_subdirectory(gatekeeper)
endif ()
# add_subdirectory(keymaster)
11 changes: 11 additions & 0 deletions seahorn/storage/CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -1,6 +1,17 @@
include_directories(include)

add_compile_definitions(CAN_RETURN_INVALID_IPC_HANDLE=1)
if (HANDLE_TYPE_IS_PTR)
set(IPC_SRC ${TRUSTY_MOD_ROOT}/trusty/user/app/storage/ipc.c)
add_compile_definitions(HANDLE_TYPE_IS_PTR=1)
else ()
set(IPC_SRC ${TRUSTY_ROOT}/trusty/user/app/storage/ipc.c)
endif ()

add_subdirectory(lib)
add_subdirectory(stubs)

# The following jobs will be built
add_subdirectory(jobs/ipc/indirect_handlers)
add_subdirectory(jobs/ipc/msg_buffer)
add_subdirectory(jobs/ipc/port_create_destroy)
3 changes: 2 additions & 1 deletion seahorn/storage/jobs/ipc/indirect_handlers/CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
add_executable(storage_ipc_indirect_handlers
${TRUSTY_ROOT}/trusty/user/app/storage/ipc.c
${IPC_SRC}
indirect_handlers_harness.c)

target_include_directories(storage_ipc_indirect_handlers
PRIVATE ${TRUSTY_ROOT}/trusty/user/app/storage
)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,9 @@

#include "ipc.h"

#include "handle_table.h"
#include <sea_handle_table.h>
#include "seahorn/seahorn.h"
#include "sea_ipc_helper.h"
#define assert sassert

#include "tipc_limits.h"
#include <interface/storage/storage.h>
Expand All @@ -34,16 +33,19 @@
entry point
*/
int main(void) {
// handle_table_init(INVALID_IPC_HANDLE, INVALID_IPC_HANDLE, INVALID_IPC_HANDLE);
struct ipc_port_context* ctx = create_port_context();
// handle_table_init(INVALID_IPC_HANDLE, INVALID_IPC_HANDLE,
// INVALID_IPC_HANDLE);
struct ipc_port_context *ctx = create_port_context();
int rc =
ipc_port_create(ctx, STORAGE_DISK_PROXY_PORT, 1, STORAGE_MAX_BUFFER_SIZE,
IPC_PORT_ALLOW_TA_CONNECT | IPC_PORT_ALLOW_NS_CONNECT);

if (rc < 0) {
return rc;
}

#if HANDLE_TYPE_IS_PTR
on_waitany_return(ctx->common.handle);
#endif
ipc_loop();

ipc_port_destroy(ctx);
Expand Down
3 changes: 2 additions & 1 deletion seahorn/storage/jobs/ipc/msg_buffer/CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
add_executable(storage_ipc_msg_buffer
${TRUSTY_ROOT}/trusty/user/app/storage/ipc.c
${IPC_SRC}
msg_buffer_harness.c)

target_include_directories(storage_ipc_msg_buffer
PRIVATE ${TRUSTY_ROOT}/trusty/user/app/storage
)
Expand Down
6 changes: 4 additions & 2 deletions seahorn/storage/jobs/ipc/msg_buffer/msg_buffer_harness.c
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
#include "tipc_limits.h"
#include <interface/storage/storage.h>

#include "handle_table.h"
#include <sea_handle_table.h>
#include "seahorn/seahorn.h"
#include "sea_ipc_helper.h"

Expand All @@ -30,7 +30,9 @@ int main(void) {
if (rc < 0) {
return rc;
}

#if HANDLE_TYPE_IS_PTR
on_waitany_return(ctx.common.handle);
#endif
ipc_loop();

ipc_port_destroy(&ctx);
Expand Down
8 changes: 6 additions & 2 deletions seahorn/storage/jobs/ipc/port_create_destroy/CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -1,7 +1,11 @@
# We verify a modified copy of ipc.c since the original
# casts between a handle_t and int.
# This breaks the original spec that handle_t is a 32-bit opaque number
# and hence breaks our impl of type(handle_t) == 32 bit pointer.
add_executable(storage_ipc_port_create_destroy
${TRUSTY_ROOT}/trusty/user/app/storage/ipc.c
${IPC_SRC}
port_create_destroy_harness.c)
target_include_directories(storage_ipc_port_create_destroy
target_include_directories(storage_ipc_port_create_destroy
PRIVATE ${TRUSTY_ROOT}/trusty/user/app/storage
)
sea_link_libraries(storage_ipc_port_create_destroy sea_storage_ipc_proofs.ir)
Expand Down
37 changes: 19 additions & 18 deletions seahorn/storage/lib/sea_ipc_helper.c
Original file line number Diff line number Diff line change
Expand Up @@ -45,25 +45,26 @@ static int sea_ipc_msg_handler(struct ipc_channel_context *context, void *msg,

static int sync_ipc_msg_handler(struct ipc_channel_context *context, void *msg,
size_t msg_size) {
uint8_t* snd_buf = malloc(sizeof(msg_size));
uint8_t* recv_buf = malloc(sizeof(msg_size));
struct iovec tx_iov = {
.iov_base = snd_buf,
.iov_len = msg_size,
};
struct iovec rx_iov = {
.iov_base = recv_buf,
.iov_len = msg_size,
};

handle_t rc = sync_ipc_send_msg(context->common.handle, &tx_iov, 1, &rx_iov, 1);

if (rc < 0)
return rc;

if (rc > 0) sassert(rc == msg_size);
uint8_t *snd_buf = malloc(sizeof(msg_size));
uint8_t *recv_buf = malloc(sizeof(msg_size));
struct iovec tx_iov = {
.iov_base = snd_buf,
.iov_len = msg_size,
};
struct iovec rx_iov = {
.iov_base = recv_buf,
.iov_len = msg_size,
};

int rc = sync_ipc_send_msg(context->common.handle, &tx_iov, 1, &rx_iov, 1);

return NO_ERROR;
if (rc < 0)
return rc;

if (rc > 0)
sassert(rc == msg_size);

return NO_ERROR;
}

/*
Expand Down
2 changes: 1 addition & 1 deletion seahorn/trusty/CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -1 +1 @@
add_subdirectory(user/base/lib/libc-trusty)
add_subdirectory(user/base/lib)
12 changes: 12 additions & 0 deletions seahorn/trusty/user/base/lib/CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
set(EXTERNAL ${TRUSTY_ROOT}/trusty/user/base/lib/libc-trusty)
add_library(
sea_libc_trusty
${EXTERNAL}/ipc.c
)

if (HANDLE_TYPE_IS_PTR)
add_subdirectory(libc-trusty-ptr)
else ()
add_subdirectory(libc-trusty)
endif ()
add_compile_definitions(CAN_RETURN_INVALID_IPC_HANDLE=0)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are there multiple conflicting compiler definitions? Are you assuming they are resolved somehow by CMake or by the compiler?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the original spec (impl), some jobs are written assuming a valid handle_t is always returned by the environment. Other jobs account for failure. I have added this compile_definition to control what the environment will return per job.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed that this definition is added multiple times in different CMakeLists.txt files. I am asking how the exclusivity of which definition is active is maintained.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The short answer is that exclusivity is maintained by user discipline. Each directory of jobs is independent of other jobs hence there should not be a conflict.

--
The problem is that this can cause silent unexpected behaviour if user breaks discipline or jobs are rearchitected to be dependent on each other. One solution is to move to the target based compilation model instead of the directory one used now. In doing so we can ensure that dependencies are explicit and there are known (few) points where a token can redefined. Right now, each directory can redefine a token which can become hard to reason about.

Changing the compilation model seems out of scope in this PR.

10 changes: 10 additions & 0 deletions seahorn/trusty/user/base/lib/libc-trusty-ptr/CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
target_sources(sea_libc_trusty PUBLIC
sea_handle_table.c
trusty_syscalls_impl.c
)

target_include_directories(sea_libc_trusty PUBLIC ${CMAKE_CURRENT_SOURCE_DIR})

sea_attach_bc(sea_libc_trusty)

add_subdirectory(proof)
11 changes: 11 additions & 0 deletions seahorn/trusty/user/base/lib/libc-trusty-ptr/proof/CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
add_subdirectory(port_create)
add_subdirectory(port_close)
add_subdirectory(port_connect)
add_subdirectory(port_accept)
add_subdirectory(port_get_msg)
add_subdirectory(port_put_msg)
add_subdirectory(port_read_msg)
add_subdirectory(port_send_msg)
add_subdirectory(port_wait)
add_subdirectory(port_wait_any)
add_subdirectory(port_set_cookie)
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
add_executable(port_accept_pf main.c)
sea_link_libraries(port_accept_pf sea_libc_trusty.ir)

sea_attach_bc(port_accept_pf)
sea_add_unsat_test(port_accept_pf)
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
#include "trusty_ipc.h"
#include <seahorn/seahorn.h>

/* Documentation from trusty API:
accept()
Accepts an incoming connection and gets a handle to a channel.
long accept(uint32_t handle_id, uuid_t *peer_uuid);
[in] handle_id: Handle representing the port to which a client has connected
[out] peer_uuid: Pointer to a uuud_t structure to be filled with the UUID of
the connecting client application. It will be set to all zeros if the
connection originated from the non-secure world.
[retval]: Handle to a channel (if non-negative) on which the server can exchange
messages with the client (or an error code otherwise)
*/

const char *HANDLE_PATH = "seahorn.com";

bool is_uuid_all_zeros(uuid_t *peer_uuid) {
return peer_uuid->time_low == 0 && peer_uuid->time_mid == 0 &&
peer_uuid->time_hi_and_version == 0;
}

int main(void) {

handle_t port;
int rc;

port = port_create(HANDLE_PATH, 1, 100,
IPC_PORT_ALLOW_NS_CONNECT | IPC_PORT_ALLOW_TA_CONNECT);

// -- got expected handle
sassert(!port->secure);

uuid_t peer_uuid;
handle_t chan;
chan = accept(port, &peer_uuid);
assume(chan != INVALID_IPC_HANDLE);
sassert(is_uuid_all_zeros(&peer_uuid));

rc = close(port);
sassert(rc == 0);

port = port_create(HANDLE_PATH, 1, 100, IPC_PORT_ALLOW_NS_CONNECT);

// -- expected secure handle handle
sassert(port->secure);

chan = accept(port, &peer_uuid);
assume(chan != INVALID_IPC_HANDLE);
return 0;
}
Loading