Skip to content

Commit

Permalink
[vmm] Avoid undefined behavior
Browse files Browse the repository at this point in the history 
Before this change UBSAN caught a misaligned pointer:

```
../../src/virtualization/bin/vmm/device/virtio_magma.cc:430:7: runtime error: reference binding to misaligned address 0x200b5fbb5114 for type 'const std::unordered_map<unsigned long, ImageInfoWithToken>::key_type' (aka 'const unsigned long'), which requires 8 byte alignment
0x200b5fbb5114: note: pointer points here
  00 00 00 00 00 39 70 06  a9 26 00 00 00 00 00 00  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00
              ^
   #0    0x0000230cbdba2eeb in VirtioMagma::Handle_virt_create_image(VirtioMagma*, virtio_magma_virt_create_image_ctrl_t const*, virtio_magma_virt_create_image_resp_t*) ../../src/virtualization/bin/vmm/device/virtio_magma.cc:430 <<application>>+0xa6eeb
   #1.2  0x000021fbe7871e37 in ubsan_GetStackTrace() compiler-rt/lib/ubsan/ubsan_diag.cpp:55 <libclang_rt.asan.so>+0x3be37
   #1.1  0x000021fbe7871e37 in MaybePrintStackTrace() compiler-rt/lib/ubsan/ubsan_diag.cpp:53 <libclang_rt.asan.so>+0x3be37
   #1    0x000021fbe7871e37 in ~ScopedReport() compiler-rt/lib/ubsan/ubsan_diag.cpp:389 <libclang_rt.asan.so>+0x3be37
   #2    0x000021fbe7872acb in handleTypeMismatchImpl() compiler-rt/lib/ubsan/ubsan_handlers.cpp:137 <libclang_rt.asan.so>+0x3cacb
   #3    0x000021fbe78725dd in compiler-rt/lib/ubsan/ubsan_handlers.cpp:142 <libclang_rt.asan.so>+0x3c5dd
   #4    0x0000230cbdba2eeb in VirtioMagma::Handle_virt_create_image(VirtioMagma*, virtio_magma_virt_create_image_ctrl_t const*, virtio_magma_virt_create_image_resp_t*) ../../src/virtualization/bin/vmm/device/virtio_magma.cc:430 <<application>>+0xa6eeb
   #5    0x0000230cbdb949ed in VirtioMagmaGeneric::HandleCommand(VirtioMagmaGeneric*, VirtioChain) x64-asan-ubsan/gen/src/virtualization/bin/vmm/device/virtio_magma_generic.h:2144 <<application>>+0x989ed
   #6    0x0000230cbdb89ba5 in VirtioMagma::NotifyQueue(VirtioMagma*, uint16_t) ../../src/virtualization/bin/vmm/device/virtio_magma.cc:131 <<application>>+0x8dba5
```

and

```
../../src/virtualization/bin/vmm/device/virtio_magma.cc:276:41: runtime error: reference binding to misaligned address 0x23b84e025134 for type 'const uint64_t' (aka 'const unsigned long'), which requires 8 byte alignment
0x23b84e025134: note: pointer points here
  48 27 00 00 00 39 4a bb  b8 26 00 00 00 00 00 00  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00
              ^
   #0    0x000021c24790cea7 in VirtioMagma::Handle_export(VirtioMagma*, virtio_magma_export_ctrl_t const*, virtio_magma_export_resp_t*) ../../src/virtualization/bin/vmm/device/virtio_magma.cc:276 <<application>>+0xa4ea7
   #1.2  0x0000208e12627e37 in ubsan_GetStackTrace() compiler-rt/lib/ubsan/ubsan_diag.cpp:55 <libclang_rt.asan.so>+0x3be37
   #1.1  0x0000208e12627e37 in MaybePrintStackTrace() compiler-rt/lib/ubsan/ubsan_diag.cpp:53 <libclang_rt.asan.so>+0x3be37
   #1    0x0000208e12627e37 in ~ScopedReport() compiler-rt/lib/ubsan/ubsan_diag.cpp:389 <libclang_rt.asan.so>+0x3be37
   #2    0x0000208e12628acb in handleTypeMismatchImpl() compiler-rt/lib/ubsan/ubsan_handlers.cpp:137 <libclang_rt.asan.so>+0x3cacb
   #3    0x0000208e126285dd in compiler-rt/lib/ubsan/ubsan_handlers.cpp:142 <libclang_rt.asan.so>+0x3c5dd
   #4    0x000021c24790cea7 in VirtioMagma::Handle_export(VirtioMagma*, virtio_magma_export_ctrl_t const*, virtio_magma_export_resp_t*) ../../src/virtualization/bin/vmm/device/virtio_magma.cc:276 <<application>>+0xa4ea7
   #5    0x000021c2478ff537 in VirtioMagmaGeneric::HandleCommand(VirtioMagmaGeneric*, VirtioChain) x64-asan-ubsan/gen/src/virtualization/bin/vmm/device/virtio_magma_generic.h:1148 <<application>>+0x97537
   #6    0x000021c2478f5ba5 in VirtioMagma::NotifyQueue(VirtioMagma*, uint16_t) ../../src/virtualization/bin/vmm/device/virtio_magma.cc:128 <<application>>+0x8dba5
```

Avoid this by stack-allocating the response type and copying it into the
destination buffer, and by making stack copies of request members before
passing them by reference.

Multiply: fuchsia.com/vmm_tests#meta/device_tests
Fixed: 66436
Change-Id: I414c2ddec2c508b28c02b8514be2482ffbc54ec3
Reviewed-on: https://fuchsia-review.googlesource.com/c/fuchsia/+/573744
Commit-Queue: Tamir Duberstein <[email protected]>
Reviewed-by: Abdulla Kamar <[email protected]>
  • Loading branch information
@tamird
tamird authored and CQ Bot committed Aug 27, 2021
1 parent 23de68a commit e7765fa
Show file tree
Hide file tree
Showing 3 changed files with 18 additions and 22 deletions.
3 changes: 0 additions & 3 deletions src/virtualization/bin/vmm/device/BUILD.gn
View file
Original file line number Diff line number Diff line change
Expand Up @@ -214,9 +214,6 @@ source_set("virtio_magma_lib") {
defines = [ "VIRTMAGMA_DEBUG=1" ]
}

# TODO(fxbug.dev/66436): Fix instances of undefined behavior.
configs += [ "//build/config:temporarily_disable_ubsan_do_not_use" ]

# TODO(https://fxbug.dev/58162): delete the below and fix compiler warnings
configs += [ "//build/config:Wno-conversion" ]
}
Expand Down
22 changes: 10 additions & 12 deletions src/virtualization/bin/vmm/device/virtio_magma.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,13 +12,10 @@
#include <lib/fit/defer.h>
#include <lib/syslog/cpp/log_settings.h>
#include <lib/syslog/cpp/macros.h>
#include <lib/syslog/global.h>
#include <lib/trace-provider/provider.h>
#include <lib/trace/event.h>
#include <lib/zx/channel.h>
#include <lib/zx/vmar.h>
#include <sys/stat.h>
#include <unistd.h>
#include <zircon/status.h>

#include "src/virtualization/bin/vmm/device/magma_image.h"
Expand Down Expand Up @@ -171,8 +168,8 @@ zx_status_t VirtioMagma::Handle_release_buffer(const virtio_magma_release_buffer

auto connection = reinterpret_cast<magma_connection_t>(request->connection);

auto& image_map = connection_image_map_[connection];
image_map.erase(request->buffer);
const uint64_t buffer = request->buffer;
connection_image_map_[connection].erase(buffer);

return ZX_OK;
}
Expand All @@ -193,17 +190,18 @@ zx_status_t VirtioMagma::Handle_internal_map(const virtio_magma_internal_map_ctr

zx::vmo vmo(handle);

const uint64_t length = request->length;
zx_vaddr_t zx_vaddr;
zx_status_t zx_status = vmar_.map(ZX_VM_PERM_READ | ZX_VM_PERM_WRITE, 0 /*vmar_offset*/, vmo,
0 /* vmo_offset */, request->length, &zx_vaddr);
0 /* vmo_offset */, length, &zx_vaddr);
if (zx_status != ZX_OK) {
FX_LOGS(ERROR) << "vmar map (length " << request->length << ") failed: " << zx_status;
FX_LOGS(ERROR) << "vmar map (length " << length << ") failed: " << zx_status;
response->result_return = MAGMA_STATUS_INVALID_ARGS;
return zx_status;
}

const uint64_t buffer_id = magma_get_buffer_id(reinterpret_cast<magma_buffer_t>(request->buffer));
buffer_maps_.emplace(buffer_id, std::pair<zx_vaddr_t, size_t>(zx_vaddr, request->length));
buffer_maps_.emplace(buffer_id, std::pair<zx_vaddr_t, size_t>(zx_vaddr, length));

response->address_out = zx_vaddr;

Expand All @@ -216,7 +214,7 @@ zx_status_t VirtioMagma::Handle_internal_unmap(const virtio_magma_internal_unmap

response->hdr.type = VIRTIO_MAGMA_RESP_INTERNAL_UNMAP;

uint64_t buffer_id = magma_get_buffer_id(reinterpret_cast<magma_buffer_t>(request->buffer));
const uint64_t buffer_id = magma_get_buffer_id(reinterpret_cast<magma_buffer_t>(request->buffer));

for (auto iter = buffer_maps_.find(buffer_id); iter != buffer_maps_.end(); iter++) {
const auto& mapping = iter->second;
Expand Down Expand Up @@ -276,7 +274,8 @@ zx_status_t VirtioMagma::Handle_export(const virtio_magma_export_ctrl_t* request
auto& image_map =
connection_image_map_[reinterpret_cast<magma_connection_t>(request->connection)];

auto iter = image_map.find(request->buffer);
const uint64_t buffer = request->buffer;
auto iter = image_map.find(buffer);
if (iter == image_map.end()) {
response->hdr.type = VIRTIO_MAGMA_RESP_EXPORT;
response->buffer_handle_out = 0;
Expand Down Expand Up @@ -366,8 +365,7 @@ zx_status_t VirtioMagma::Handle_import(const virtio_magma_import_ctrl_t* request
auto& image_map =
connection_image_map_[reinterpret_cast<magma_connection_t>(request->connection)];

magma_buffer_t image = response->buffer_out;
image_map[image] = std::move(info);
image_map[response->buffer_out] = std::move(info);
}

return ZX_OK;
Expand Down
15 changes: 8 additions & 7 deletions src/virtualization/bin/vmm/device/virtio_magma_generic_h_gen.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,7 @@ def generate_handle_command(magma):
ret += ' case VIRTIO_MAGMA_CMD_' + name.upper() + ': {\n'
ret += ' TRACE_DURATION("magma", "' + name + '");\n'
ret += ' auto request = reinterpret_cast<const virtio_magma_' + name + '_ctrl_t*>(request_desc.addr);\n'
ret += ' auto response = reinterpret_cast<virtio_magma_' + name + '_resp_t*>(response_desc.addr);\n'
ret += ' virtio_magma_' + name + '_resp_t response{};\n'
ret += '#ifdef VIRTMAGMA_DEBUG\n'
ret += ' FX_LOGS(INFO) << "Received MAGMA command (" << command_type << "):\\n"\\\n'
ret += ' " hdr = { " << virtio_magma_ctrl_type_string((virtio_magma_ctrl_type)request->hdr.type) << ", " << request->hdr.flags << " }"\\\n'
Expand All @@ -171,30 +171,31 @@ def generate_handle_command(magma):
'name'] + ') << ""\\\n'
ret += ' "";\n'
ret += '#endif // VIRTMAGMA_DEBUG\n'
ret += ' if (response_desc.len < sizeof(*response)) {\n'
ret += ' if (response_desc.len < sizeof(response)) {\n'
ret += ' FX_LOGS(ERROR) << "MAGMA command (" << command_type << ") response descriptor too small";\n'
ret += ' chain.Return();\n'
ret += ' return ZX_ERR_INVALID_ARGS;\n'
ret += ' }\n'
ret += ' zx_status_t status = Handle_' + name + '(request, response);\n'
ret += ' zx_status_t status = Handle_' + name + '(request, &response);\n'
ret += ' if (status != ZX_OK) {\n'
ret += ' FX_LOGS(ERROR) << "Handle_' + name + ' failed (" << zx_status_get_string(status) << ")";\n'
ret += ' chain.Return();\n'
ret += ' return status;\n'
ret += ' }\n'
ret += '#ifdef VIRTMAGMA_DEBUG\n'
ret += ' FX_LOGS(INFO) << "Sending MAGMA response:\\n"\\\n'
ret += ' " hdr = { " << virtio_magma_ctrl_type_string((virtio_magma_ctrl_type)response->hdr.type) << ", " << response->hdr.flags << " }"\\\n'
ret += ' " hdr = { " << virtio_magma_ctrl_type_string((virtio_magma_ctrl_type)response.hdr.type) << ", " << response.hdr.flags << " }"\\\n'
for argument in export['arguments']:
if is_response_argument(argument):
ret += ' "\\n ' + argument[
'name'] + ' = " << static_cast<uint64_t>(response->' + argument[
'name'] + ' = " << static_cast<uint64_t>(response.' + argument[
'name'] + ') << ""\\\n'
if export['type'] != 'void':
ret += ' "\\n result_return = " << static_cast<int64_t>(response->result_return) << ""\\\n'
ret += ' "\\n result_return = " << static_cast<int64_t>(response.result_return) << ""\\\n'
ret += ' "";\n'
ret += '#endif // VIRTMAGMA_DEBUG\n'
ret += ' *chain.Used() = sizeof(*response);\n'
ret += ' memcpy(response_desc.addr, &response, sizeof(response));\n'
ret += ' *chain.Used() = sizeof(response);\n'
ret += ' } break;\n'
ret += ''' default: {
FX_LOGS(ERROR) << "Unsupported MAGMA command (" << command_type << ")";
Expand Down

0 comments on commit e7765fa

Please sign in to comment.