Check program state before permitting linking (#2250)

* Check program state before permitting linking

Signed-off-by: Alan Jowett <alanjo@microsoft.com>

* PR feedback

Signed-off-by: Alan Jowett <alanjo@microsoft.com>

* Don't return pointer to mutable state

Signed-off-by: Alan Jowett <alanjo@microsoft.com>

* Fix code analysis failure

Signed-off-by: Alan Jowett <alanjo@microsoft.com>

* Update libs/execution_context/ebpf_program.h

Co-authored-by: Dave Thaler <dthaler@microsoft.com>

* PR feedback

Signed-off-by: Alan Jowett <alanjo@microsoft.com>

* mend

---------

Signed-off-by: Alan Jowett <alanjo@microsoft.com>
Co-authored-by: Dave Thaler <dthaler@microsoft.com>

libs/execution_context/ebpf_core.c

@@ -1017,17 +1017,28 @@ _ebpf_core_protocol_query_program_info(
     ebpf_result_t retval;
     ebpf_program_t* program = NULL;
     size_t required_reply_length;
-    const ebpf_program_parameters_t* parameters;
+    ebpf_utf8_string_t file_name = {0};
+    ebpf_utf8_string_t section_name = {0};
+    ebpf_code_type_t code_type;
 
     retval = ebpf_object_reference_by_handle(request->handle, EBPF_OBJECT_PROGRAM, (ebpf_core_object_t**)&program);
     if (retval != EBPF_SUCCESS) {
         goto Done;
     }
 
-    parameters = ebpf_program_get_parameters(program);
+    retval = ebpf_program_get_program_file_name(program, &file_name);
+    if (retval != EBPF_SUCCESS) {
+        goto Done;
+    }
 
-    retval =
+    retval = ebpf_program_get_program_section_name(program, &section_name);
-        ebpf_safe_size_t_add(parameters->section_name.length, parameters->file_name.length, &required_reply_length);
+    if (retval != EBPF_SUCCESS) {
+        goto Done;
+    }
+
+    code_type = ebpf_program_get_code_type(program);
+
+    retval = ebpf_safe_size_t_add(section_name.length, file_name.length, &required_reply_length);
     if (retval != EBPF_SUCCESS) {
         goto Done;
     }
@@ -1045,15 +1056,18 @@ _ebpf_core_protocol_query_program_info(
     }
 
     reply->file_name_offset = EBPF_OFFSET_OF(struct _ebpf_operation_query_program_info_reply, data);
-    reply->section_name_offset = reply->file_name_offset + (uint16_t)parameters->file_name.length;
+    reply->section_name_offset = reply->file_name_offset + (uint16_t)file_name.length;
 
-    memcpy(reply->data, parameters->file_name.value, parameters->file_name.length);
+    memcpy(reply->data, file_name.value, file_name.length);
-    memcpy(reply->data + parameters->file_name.length, parameters->section_name.value, parameters->section_name.length);
+    memcpy(reply->data + file_name.length, section_name.value, section_name.length);
-    reply->code_type = parameters->code_type;
+    reply->code_type = code_type;
 
     reply->header.length = (uint16_t)required_reply_length;
 
 Done:
+    ebpf_utf8_string_free(&file_name);
+    ebpf_utf8_string_free(&section_name);
+
     ebpf_object_release_reference((ebpf_core_object_t*)program);
 
     EBPF_RETURN_RESULT(retval);
@@ -1160,6 +1174,7 @@ _ebpf_core_protocol_link_program(
     ebpf_result_t retval;
     ebpf_program_t* program = NULL;
     ebpf_link_t* link = NULL;
+    ebpf_code_type_t code_type;
 
     retval =
         ebpf_object_reference_by_handle(request->program_handle, EBPF_OBJECT_PROGRAM, (ebpf_core_object_t**)&program);
@@ -1167,6 +1182,12 @@ _ebpf_core_protocol_link_program(
         goto Done;
     }
 
+    code_type = ebpf_program_get_code_type(program);
+    if (code_type == EBPF_CODE_NONE) {
+        retval = EBPF_INVALID_ARGUMENT;
+        goto Done;
+    }
+
     retval = ebpf_link_create(&link);
     if (retval != EBPF_SUCCESS) {
         goto Done;

libs/execution_context/ebpf_link.c

@@ -166,18 +166,18 @@ _ebpf_link_extension_changed_callback(
         goto Done;
     }
 
-    const ebpf_program_type_t* program_type = ebpf_program_type_uuid(link->program);
+    ebpf_program_type_t program_type = ebpf_program_type_uuid(link->program);
     ebpf_attach_provider_data_t* attach_provider_data = (ebpf_attach_provider_data_t*)provider_data->data;
 
     if (memcmp(
-            program_type,
+            &program_type,
             &attach_provider_data->supported_program_type,
             sizeof(attach_provider_data->supported_program_type)) != 0) {
         EBPF_LOG_MESSAGE_GUID(
             EBPF_TRACELOG_LEVEL_ERROR,
             EBPF_TRACELOG_KEYWORD_LINK,
             "Attach failed due to incorrect program type",
-            *program_type);
+            program_type);
         result = EBPF_INVALID_ARGUMENT;
         goto Done;
     }

libs/execution_context/ebpf_maps.c

@@ -241,11 +241,11 @@ Done:
     return return_value;
 }
 
-_Ret_notnull_ static const ebpf_program_type_t*
+static ebpf_program_type_t
 _get_map_program_type(_In_ const ebpf_core_object_t* object)
 {
     const ebpf_core_object_map_t* map = (const ebpf_core_object_map_t*)object;
-    return &map->program_type;
+    return map->program_type;
 }
 
 typedef struct _ebpf_map_metadata_table
@@ -550,15 +550,15 @@ _associate_program_with_prog_array_map(_Inout_ ebpf_core_map_t* map, _In_ const
 
     // Validate that the program type is
     // not in conflict with the map's program type.
-    const ebpf_program_type_t* program_type = ebpf_program_type_uuid(program);
+    ebpf_program_type_t program_type = ebpf_program_type_uuid(program);
     ebpf_result_t result = EBPF_SUCCESS;
 
     ebpf_lock_state_t lock_state = ebpf_lock_lock(&program_array->lock);
 
     if (!program_array->is_program_type_set) {
         program_array->is_program_type_set = TRUE;
-        program_array->program_type = *program_type;
+        program_array->program_type = program_type;
-    } else if (memcmp(&program_array->program_type, program_type, sizeof(*program_type)) != 0) {
+    } else if (memcmp(&program_array->program_type, &program_type, sizeof(program_type)) != 0) {
         result = EBPF_INVALID_FD;
     }
 
@@ -597,8 +597,13 @@ static _Requires_lock_held_(object_map->lock) ebpf_result_t _validate_map_value_
     ebpf_result_t result = EBPF_SUCCESS;
     const ebpf_core_map_t* map = &object_map->core_map;
 
-    const ebpf_program_type_t* value_program_type =
+    ebpf_program_type_t value_program_type = {0};
-        (value_object->get_program_type) ? value_object->get_program_type(value_object) : NULL;
+    bool is_program_type_set = false;
+
+    if (value_object->get_program_type) {
+        value_program_type = value_object->get_program_type(value_object);
+        is_program_type_set = true;
+    }
 
     if (value_type == EBPF_OBJECT_MAP) {
         // Validate that the value is of the correct type.
@@ -610,11 +615,11 @@ static _Requires_lock_held_(object_map->lock) ebpf_result_t _validate_map_value_
 
     // Validate that the value's program type (if any) is
     // not in conflict with the map's program type.
-    if (value_program_type != NULL) {
+    if (is_program_type_set) {
         if (!object_map->is_program_type_set) {
             object_map->is_program_type_set = TRUE;
-            object_map->program_type = *value_program_type;
+            object_map->program_type = value_program_type;
-        } else if (memcmp(&object_map->program_type, value_program_type, sizeof(*value_program_type)) != 0) {
+        } else if (memcmp(&object_map->program_type, &value_program_type, sizeof(value_program_type)) != 0) {
             result = EBPF_INVALID_FD;
             goto Error;
         }

libs/execution_context/ebpf_program.c

@@ -91,9 +91,6 @@ _ebpf_program_update_interpret_helpers(_Inout_ ebpf_program_t* program, _Inout_
 static ebpf_result_t
 _ebpf_program_update_jit_helpers(_Inout_ ebpf_program_t* program, _Inout_ void* context);
 
-static ebpf_result_t
-_ebpf_program_register_helpers(_In_ const ebpf_program_t* program);
-
 static ebpf_result_t
 _ebpf_program_get_helper_function_address(
     _In_ const ebpf_program_t* program, const uint32_t helper_function_id, uint64_t* address);
@@ -259,7 +256,7 @@ _ebpf_program_free(_In_opt_ _Post_invalid_ ebpf_core_object_t* object)
     EBPF_RETURN_VOID();
 }
 
-static const ebpf_program_type_t*
+static ebpf_program_type_t
 _ebpf_program_get_program_type(_In_ const ebpf_core_object_t* object)
 {
     return ebpf_program_type_uuid((const ebpf_program_t*)object);
@@ -324,7 +321,7 @@ _ebpf_program_epoch_free(_In_ _Post_invalid_ void* context)
 }
 
 static ebpf_result_t
-ebpf_program_load_providers(_Inout_ ebpf_program_t* program)
+_ebpf_program_load_providers(_Inout_ ebpf_program_t* program)
 {
     EBPF_LOG_ENTRY();
     ebpf_result_t return_value;
@@ -472,6 +469,9 @@ ebpf_program_initialize(_Inout_ ebpf_program_t* program, _In_ const ebpf_program
     ebpf_utf8_string_t local_file_name = {NULL, 0};
     uint8_t* local_program_info_hash = NULL;
 
+    ebpf_lock_state_t state = ebpf_lock_lock(&program->lock);
+    bool lock_held = true;
+
     if (program->parameters.code_type != EBPF_CODE_NONE) {
         EBPF_LOG_MESSAGE_UINT64(
             EBPF_TRACELOG_LEVEL_ERROR,
@@ -533,7 +533,10 @@ ebpf_program_initialize(_Inout_ ebpf_program_t* program, _In_ const ebpf_program
     program->parameters.program_info_hash = local_program_info_hash;
     local_program_info_hash = NULL;
 
-    return_value = ebpf_program_load_providers(program);
+    ebpf_lock_unlock(&program->lock, state);
+    lock_held = false;
+
+    return_value = _ebpf_program_load_providers(program);
     if (return_value != EBPF_SUCCESS) {
         goto Done;
     }
@@ -545,25 +548,28 @@ Done:
     ebpf_free(local_program_name.value);
     ebpf_free(local_section_name.value);
     ebpf_free(local_file_name.value);
+    if (lock_held) {
+        ebpf_lock_unlock(&program->lock, state);
+    }
     EBPF_RETURN_RESULT(return_value);
 }
 
-_Ret_notnull_ const ebpf_program_parameters_t*
+ebpf_program_type_t
-ebpf_program_get_parameters(_In_ const ebpf_program_t* program)
-{
-    return &program->parameters;
-}
-
-_Ret_notnull_ const ebpf_program_type_t*
 ebpf_program_type_uuid(_In_ const ebpf_program_t* program)
 {
-    return &ebpf_program_get_parameters(program)->program_type;
+    ebpf_lock_state_t state = ebpf_lock_lock((ebpf_lock_t*)&program->lock);
+    ebpf_program_type_t return_value = program->parameters.program_type;
+    ebpf_lock_unlock((ebpf_lock_t*)&program->lock, state);
+    return return_value;
 }
 
-_Ret_notnull_ const ebpf_attach_type_t*
+ebpf_attach_type_t
 ebpf_expected_attach_type(_In_ const ebpf_program_t* program)
 {
-    return &ebpf_program_get_parameters(program)->expected_attach_type;
+    ebpf_lock_state_t state = ebpf_lock_lock((ebpf_lock_t*)&program->lock);
+    ebpf_attach_type_t return_value = program->parameters.expected_attach_type;
+    ebpf_lock_unlock((ebpf_lock_t*)&program->lock, state);
+    return return_value;
 }
 
 _Must_inspect_result_ ebpf_result_t
@@ -600,35 +606,42 @@ Done:
 _Must_inspect_result_ ebpf_result_t
 ebpf_program_associate_maps(ebpf_program_t* program, ebpf_map_t** maps, uint32_t maps_count)
 {
+    ebpf_result_t result = EBPF_SUCCESS;
     EBPF_LOG_ENTRY();
+
     size_t index;
     ebpf_map_t** program_maps = ebpf_allocate_with_tag(maps_count * sizeof(ebpf_map_t*), EBPF_POOL_TAG_PROGRAM);
     if (!program_maps) {
-        return EBPF_NO_MEMORY;
+        result = EBPF_NO_MEMORY;
+        goto Done;
     }
 
     memcpy(program_maps, maps, sizeof(ebpf_map_t*) * maps_count);
 
     // Before we acquire any references, make sure
     // all maps can be associated.
-    ebpf_result_t result = EBPF_SUCCESS;
     for (index = 0; index < maps_count; index++) {
         ebpf_map_t* map = program_maps[index];
         result = ebpf_map_associate_program(map, program);
         if (result != EBPF_SUCCESS) {
-            ebpf_free(program_maps);
+            goto Done;
-            EBPF_RETURN_RESULT(result);
         }
     }
 
+    ebpf_lock_state_t state = ebpf_lock_lock(&program->lock);
     // Now go through again and acquire references.
     program->maps = program_maps;
+    program_maps = NULL;
     program->count_of_maps = maps_count;
     for (index = 0; index < maps_count; index++) {
-        ebpf_object_acquire_reference((ebpf_core_object_t*)program_maps[index]);
+        ebpf_object_acquire_reference((ebpf_core_object_t*)program->maps[index]);
     }
+    ebpf_lock_unlock(&program->lock, state);
 
-    EBPF_RETURN_RESULT(EBPF_SUCCESS);
+Done:
+    ebpf_free(program_maps);
+
+    EBPF_RETURN_RESULT(result);
 }
 
 static ebpf_result_t
@@ -1477,8 +1490,8 @@ ebpf_program_get_info(
     output_info->nr_map_ids = program->count_of_maps;
     output_info->map_ids = (uintptr_t)map_ids;
     output_info->type = _ebpf_program_get_bpf_prog_type(program);
-    output_info->type_uuid = *ebpf_program_type_uuid(program);
+    output_info->type_uuid = ebpf_program_type_uuid(program);
-    output_info->attach_type_uuid = *ebpf_expected_attach_type(program);
+    output_info->attach_type_uuid = ebpf_expected_attach_type(program);
     output_info->pinned_path_count = program->object.pinned_path_count;
     output_info->link_count = program->link_count;
 
@@ -1934,3 +1947,30 @@ ebpf_program_register_for_helper_changes(
     program->helper_function_addresses_changed_context = context;
     return EBPF_SUCCESS;
 }
+
+_Must_inspect_result_ ebpf_result_t
+ebpf_program_get_program_file_name(_In_ const ebpf_program_t* program, _Out_ ebpf_utf8_string_t* file_name)
+{
+    ebpf_lock_state_t state = ebpf_lock_lock((ebpf_lock_t*)&program->lock);
+    ebpf_result_t return_value = ebpf_duplicate_utf8_string(file_name, &program->parameters.file_name);
+    ebpf_lock_unlock((ebpf_lock_t*)&program->lock, state);
+    return return_value;
+}
+
+_Must_inspect_result_ ebpf_result_t
+ebpf_program_get_program_section_name(_In_ const ebpf_program_t* program, _Out_ ebpf_utf8_string_t* section_name)
+{
+    ebpf_lock_state_t state = ebpf_lock_lock((ebpf_lock_t*)&program->lock);
+    ebpf_result_t return_value = ebpf_duplicate_utf8_string(section_name, &program->parameters.section_name);
+    ebpf_lock_unlock((ebpf_lock_t*)&program->lock, state);
+    return return_value;
+}
+
+ebpf_code_type_t
+ebpf_program_get_code_type(_In_ const ebpf_program_t* program)
+{
+    ebpf_lock_state_t state = ebpf_lock_lock((ebpf_lock_t*)&program->lock);
+    ebpf_code_type_t code_type = program->parameters.code_type;
+    ebpf_lock_unlock((ebpf_lock_t*)&program->lock, state);
+    return code_type;
+}

libs/execution_context/ebpf_program.h

@@ -81,18 +81,40 @@ extern "C"
     ebpf_program_initialize(_Inout_ ebpf_program_t* program, _In_ const ebpf_program_parameters_t* program_parameters);
 
     /**
-     * @brief Get parameters describing the program instance.
+     * @brief Get the original file name of the program.
      *
-     * @param[in] program Program instance to query.
+     * @param[in] program The program instance.
-     * @returns Pointer to parameters of the program.
+     * @param[out] file_name The file name of the program. Caller must free this.
+     * @retval EBPF_SUCCESS The operation was successful.
+     * @retval EBPF_NO_MEMORY Unable to allocate resources.
      */
-    _Ret_notnull_ const ebpf_program_parameters_t*
+    _Must_inspect_result_ ebpf_result_t
-    ebpf_program_get_parameters(_In_ const ebpf_program_t* program);
+    ebpf_program_get_program_file_name(_In_ const ebpf_program_t* program, _Out_ ebpf_utf8_string_t* file_name);
 
-    _Ret_notnull_ const ebpf_program_type_t*
+    /**
+     * @brief Get the original section name of the program.
+     *
+     * @param[in] program The program instance.
+     * @param[out] section_name The section name of the program. Caller must free this.
+     * @retval EBPF_SUCCESS The operation was successful.
+     * @retval EBPF_NO_MEMORY Unable to allocate resources.
+     */
+    _Must_inspect_result_ ebpf_result_t
+    ebpf_program_get_program_section_name(_In_ const ebpf_program_t* program, _Out_ ebpf_utf8_string_t* section_name);
+
+    /**
+     * @brief Get the code type of the program.
+     *
+     * @param[in] program The program instance.
+     * @return The code type of the program.
+     */
+    ebpf_code_type_t
+    ebpf_program_get_code_type(_In_ const ebpf_program_t* program);
+
+    ebpf_program_type_t
     ebpf_program_type_uuid(_In_ const ebpf_program_t* program);
 
-    _Ret_notnull_ const ebpf_attach_type_t*
+    ebpf_attach_type_t
     ebpf_expected_attach_type(_In_ const ebpf_program_t* program);
 
     /**

libs/execution_context/unit/execution_context_unit_test.cpp

@@ -679,9 +679,9 @@ TEST_CASE("program", "[execution_context]")
 
     REQUIRE(ebpf_program_initialize(program.get(), &program_parameters) == EBPF_SUCCESS);
 
-    const ebpf_program_type_t* returned_program_type = ebpf_program_type_uuid(program.get());
+    ebpf_program_type_t returned_program_type = ebpf_program_type_uuid(program.get());
     REQUIRE(
-        memcmp(&program_parameters.program_type, returned_program_type, sizeof(program_parameters.program_type)) == 0);
+        memcmp(&program_parameters.program_type, &returned_program_type, sizeof(program_parameters.program_type)) == 0);
 
     REQUIRE(ebpf_program_get_program_info(program.get(), &program_info) == EBPF_SUCCESS);
     REQUIRE(program_info != nullptr);
@@ -1654,7 +1654,7 @@ TEST_CASE("EBPF_OPERATION_LINK_PROGRAM", "[execution_context][negative]")
 
     // No provider.
     link_program_request->program_handle = program_handles[0];
-    REQUIRE(invoke_protocol(EBPF_OPERATION_LINK_PROGRAM, request, reply) == EBPF_EXTENSION_FAILED_TO_LOAD);
+    REQUIRE(invoke_protocol(EBPF_OPERATION_LINK_PROGRAM, request, reply) == EBPF_INVALID_ARGUMENT);
 }
 
 TEST_CASE("EBPF_OPERATION_GET_EC_FUNCTION", "[execution_context][negative]")

libs/platform/ebpf_object.c

@@ -263,6 +263,14 @@ ebpf_duplicate_utf8_string(_Out_ ebpf_utf8_string_t* destination, _In_ const ebp
     }
 }
 
+void
+ebpf_utf8_string_free(_Inout_ ebpf_utf8_string_t* string)
+{
+    ebpf_free(string->value);
+    string->value = NULL;
+    string->length = 0;
+}
+
 _Requires_lock_held_(&_ebpf_object_tracking_list_lock) static ebpf_core_object_t* _get_next_object_by_id(
     ebpf_id_t start_id, ebpf_object_type_t object_type)
 {

libs/platform/ebpf_object.h

@@ -23,7 +23,7 @@ extern "C"
 
     typedef struct _ebpf_core_object ebpf_core_object_t;
     typedef void (*ebpf_free_object_t)(ebpf_core_object_t* object);
-    typedef const ebpf_program_type_t* (*ebpf_object_get_program_type_t)(_In_ const ebpf_core_object_t* object);
+    typedef const ebpf_program_type_t (*ebpf_object_get_program_type_t)(_In_ const ebpf_core_object_t* object);
 
     typedef struct _ebpf_base_object
     {

libs/platform/ebpf_platform.h

@@ -303,6 +303,14 @@ extern "C"
     _Must_inspect_result_ ebpf_result_t
     ebpf_duplicate_utf8_string(_Out_ ebpf_utf8_string_t* destination, _In_ const ebpf_utf8_string_t* source);
 
+    /**
+     * @brief Free a UTF-8 string allocated by ebpf_duplicate_utf8_string.
+     *
+     * @param[in,out] string The string to free.
+     */
+    void
+    ebpf_utf8_string_free(_Inout_ ebpf_utf8_string_t* string);
+
     /**
      * @brief Duplicate a null-terminated string.
      *

netebpfext/net_ebpf_ext_sock_addr.c

@@ -1542,7 +1542,7 @@ Exit:
     if (classify_handle_acquired) {
         FwpsReleaseClassifyHandle(classify_handle);
     }
-    
+
     if (attached_client) {
         net_ebpf_extension_hook_client_leave_rundown(attached_client);
     }