Kernel: Make UserOrKernelBuffer a lot more safe to use #25640
Conversation
First, we should not allow passing a pointer to UserOrKernelBuffer in the BlockBasedFileSystem code, so instead require to use Optional. To add memory safety to this component, we require instances to hold the size of the buffer, so offsetting outside of a buffer is no longer possible. In addition to that, instantiating a new UserOrKernelBuffer is a lot more straightforward, as it can be generated from many AK containers which ensures memory safety by using the actual type or its properties to acquire the buffer size.
| @@ -94,7 +94,7 @@ ErrorOr<size_t> TTY::write(OpenFileDescription&, u64, UserOrKernelBuffer const& | |||
| modified_data[modified_data_size++] = out_ch; | |||
| }); | |||
| } | |||
| auto bytes_written_or_error = on_tty_write(UserOrKernelBuffer::for_kernel_buffer(modified_data), modified_data_size); | |||
| auto bytes_written_or_error = on_tty_write(UserOrKernelBuffer::for_kernel_buffer(modified_data, num_chars * 2), modified_data_size); | |||
There was a problem hiding this comment.
It might be nicer to use sizeof() on the actual data, where possible, instead of duplicating the size calculation or using sizeof(StructName).
| { | ||
| VERIFY(!kernel_buffer || !Memory::is_user_address(VirtualAddress(kernel_buffer))); | ||
| return UserOrKernelBuffer(kernel_buffer); | ||
| VERIFY(!Memory::is_user_address(VirtualAddress(bytes.data())) || !Memory::is_user_address(VirtualAddress(bytes.data()).offset(bytes.size()))); |
There was a problem hiding this comment.
Shouldn't all of these conditions be VERIFY(!bytes.is_empty() && !Memory::is_user_address(VirtualAddress(bytes.data())) && !Memory::is_user_address(VirtualAddress(bytes.data()).offset(bytes.size() - 1)));?
And is there a reason why we can't just use is_user_range here?
| @@ -20,36 +22,60 @@ class [[nodiscard]] UserOrKernelBuffer { | |||
| public: | |||
| UserOrKernelBuffer() = delete; | |||
|
|
|||
| static UserOrKernelBuffer for_kernel_buffer(u8* kernel_buffer) | |||
| static UserOrKernelBuffer for_kernel_buffer(ReadonlyBytes bytes) | |||
There was a problem hiding this comment.
All of these read-only const_casting overloads don't seem very nice, do we really need them?
It might be nice to somehow make UserOrKernelBuffer const-correct in the future.
| @@ -768,7 +768,7 @@ static ErrorOr<FixedArray<u8>> read_elf_buffer_including_program_headers(OpenFil | |||
| return EINVAL; | |||
|
|
|||
| auto elf_buffer = TRY(FixedArray<u8>::create(last_needed_byte_offset_on_program_header_list)); | |||
| auto elf_read_buffer = UserOrKernelBuffer::for_kernel_buffer(elf_buffer.data()); | |||
| auto elf_read_buffer = UserOrKernelBuffer::for_kernel_buffer(elf_buffer.data(), last_needed_byte_offset_on_program_header_list); | |||
There was a problem hiding this comment.
Can this be for_kernel_buffer(elf_buffer.span()) instead?
| } | ||
|
|
||
| return {}; | ||
| } | ||
|
|
||
| ErrorOr<void> Coredump::write_notes_segment(ReadonlyBytes notes_segment) | ||
| { | ||
| TRY(m_description->write(UserOrKernelBuffer::for_kernel_buffer(const_cast<u8*>(notes_segment.data())), notes_segment.size())); | ||
| TRY(m_description->write(UserOrKernelBuffer::for_kernel_buffer(const_cast<u8*>(notes_segment.data()), notes_segment.size()), notes_segment.size())); |
There was a problem hiding this comment.
Can't this just be for_kernel_buffer(notes_segment)?
| @@ -750,7 +750,7 @@ ErrorOr<void> Ext2FS::flush_writes() | |||
| } | |||
| for (auto& cached_bitmap : m_cached_bitmaps) { | |||
| if (cached_bitmap->dirty) { | |||
| auto buffer = UserOrKernelBuffer::for_kernel_buffer(cached_bitmap->buffer->data()); | |||
| auto buffer = UserOrKernelBuffer::for_kernel_buffer(cached_bitmap->buffer->data(), cached_bitmap->buffer->size()); | |||
There was a problem hiding this comment.
UserOrKernelBuffer::for_kernel_buffer(*cached_bitmap->buffer)?
| @@ -796,12 +796,12 @@ ErrorOr<NonnullRefPtr<Ext2FSInode>> Ext2FS::build_root_inode() const | |||
|
|
|||
| auto inode = TRY(adopt_nonnull_ref_or_enomem(new (nothrow) Ext2FSInode(const_cast<Ext2FS&>(*this), EXT2_ROOT_INO))); | |||
|
|
|||
| auto buffer = UserOrKernelBuffer::for_kernel_buffer(reinterpret_cast<u8*>(&inode->m_raw_inode)); | |||
| auto buffer = UserOrKernelBuffer::for_kernel_buffer(reinterpret_cast<u8*>(&inode->m_raw_inode), sizeof(ext2_inode_large)); | |||
There was a problem hiding this comment.
Shouldn't the size be the same as calculated in the next line?
| @@ -837,12 +837,12 @@ ErrorOr<NonnullRefPtr<Inode>> Ext2FS::get_inode(InodeIdentifier inode) const | |||
|
|
|||
| auto new_inode = TRY(adopt_nonnull_ref_or_enomem(new (nothrow) Ext2FSInode(const_cast<Ext2FS&>(*this), inode.index()))); | |||
|
|
|||
| auto buffer = UserOrKernelBuffer::for_kernel_buffer(reinterpret_cast<u8*>(&new_inode->m_raw_inode)); | |||
| auto buffer = UserOrKernelBuffer::for_kernel_buffer(reinterpret_cast<u8*>(&new_inode->m_raw_inode), sizeof(ext2_inode_large)); | |||
| } | ||
| return {}; | ||
| } | ||
|
|
||
| ErrorOr<void> BlockBasedFileSystem::write_blocks(BlockIndex index, unsigned count, UserOrKernelBuffer const& data, bool allow_cache) | ||
| { | ||
| VERIFY(m_device_block_size); | ||
| dbgln_if(BBFS_DEBUG, "BlockBasedFileSystem::write_blocks {}, count={}", index, count); | ||
| VERIFY(count > 0); | ||
| VERIFY(((count - 1) * logical_block_size()) <= data.size()); |
There was a problem hiding this comment.
Why do you subtract 1 here? Same for read_blocks
| @@ -155,7 +155,7 @@ ErrorOr<void> BlockBasedFileSystem::write_block(BlockIndex index, UserOrKernelBu | |||
| // NOTE: We copy the `data` to write into a local buffer before taking the cache lock. | |||
| // This makes sure any page faults caused by accessing the data will occur before | |||
| // we tie down the cache. | |||
| auto buffered_data = TRY(ByteBuffer::create_uninitialized(count)); | |||
| auto buffered_data = TRY(ByteBuffer::create_uninitialized(min(count, data.size()))); | |||
There was a problem hiding this comment.
I don't really like that we are truncating the passed count here.
Can we maybe completely get rid of this argument somehow and just use the size of the passed buffer?
Especially since the memcpy down below still copies count bytes.
The OpenFileDescription::write also still uses count.
First, we should not allow passing a pointer to UserOrKernelBuffer in the BlockBasedFileSystem code, so instead require to use Optional.
To add memory safety to this component, we require instances to hold the size of the buffer, so offsetting outside of a buffer is no longer possible.
In addition to that, instantiating a new UserOrKernelBuffer is a lot more straightforward, as it can be generated from many AK containers which ensures memory safety by using the actual type or its properties to acquire the buffer size.
Fixes #25547.