Hi,
When I try to start a VirtualBox virtual machine running kernel 6.5-rc2, it gets
a kernel bug as follows while trying to mount a vboxsf-shared mount:
Jul 19 08:48:19 localhost kernel: detected buffer overflow in strscpy
Jul 19 08:48:19 localhost kernel: ------------[ cut here ]------------
Jul 19 08:48:19 localhost kernel: kernel BUG at lib/string_helpers.c:1031!
Jul 19 08:48:19 localhost kernel: invalid opcode: 0000 [#1] PREEMPT SMP PTI
Jul 19 08:48:19 localhost kernel: CPU: 2 PID: 507 Comm: mount.vboxsf Not tainted
6.5.0-rc2-1.gc159bc5-default #1 openSUSE Tumbleweed (unreleased)
4ec79f0435e04b515527e428ee3d62093b7f6e02
Jul 19 08:48:19 localhost kernel: Hardware name: innotek GmbH
VirtualBox/VirtualBox, BIOS VirtualBox 12/01/2006
Jul 19 08:48:19 localhost kernel: RIP: 0010:fortify_panic+0x13/0x20
Jul 19 08:48:19 localhost kernel: Code: 41 5d c3 cc cc cc cc 90 90 90 90 90 90
90 90 90 90 90 90 90 90 90 90 f3 0f 1e fa 48 89 fe 48 c7 c7 68 e5 c6 93 e8 ed a7
b2 ff <0f> 0b 66 66 2e 0f 1f 84 00 00 00 00 00 90 90 90 90 90 90 90 90 90
Jul 19 08:48:19 localhost kernel: RSP: 0018:ffffa72a4047fd40 EFLAGS: 00010246
Jul 19 08:48:19 localhost kernel: RAX: 0000000000000023 RBX: ffff903ac9d8ac00
RCX: 0000000000000000
Jul 19 08:48:19 localhost kernel: RDX: 0000000000000000 RSI: ffff903adbd274c0
RDI: ffff903adbd274c0
Jul 19 08:48:19 localhost kernel: RBP: 0000000000000002 R08: 0000000000000000
R09: ffffa72a4047fc00
Jul 19 08:48:19 localhost kernel: R10: 0000000000000003 R11: ffffffff94558568
R12: ffff903ac2013f00
Jul 19 08:48:19 localhost kernel: R13: ffff903ac8492800 R14: ffff903ac7b9dd20
R15: 0000000000000007
Jul 19 08:48:19 localhost kernel: FS: 00007f22f0cec740(0000)
GS:ffff903adbd00000(0000) knlGS:0000000000000000
Jul 19 08:48:19 localhost kernel: CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
Jul 19 08:48:19 localhost kernel: CR2: 00007f22f0e7f574 CR3: 0000000109474001
CR4: 00000000000706e0
Jul 19 08:48:19 localhost kernel: Call Trace:
Jul 19 08:48:19 localhost kernel: <TASK>
Jul 19 08:48:19 localhost kernel: ? die+0x36/0x90
Jul 19 08:48:19 localhost kernel: ? do_trap+0xda/0x100
Jul 19 08:48:19 localhost kernel: ? fortify_panic+0x13/0x20
Jul 19 08:48:19 localhost kernel: ? do_error_trap+0x6a/0x90
Jul 19 08:48:19 localhost kernel: ? fortify_panic+0x13/0x20
Jul 19 08:48:19 localhost kernel: ? exc_invalid_op+0x50/0x70
Jul 19 08:48:19 localhost kernel: ? fortify_panic+0x13/0x20
Jul 19 08:48:19 localhost kernel: ? asm_exc_invalid_op+0x1a/0x20
Jul 19 08:48:19 localhost kernel: ? fortify_panic+0x13/0x20
Jul 19 08:48:19 localhost kernel: vboxsf_fill_super+0x3bc/0x3c0 [vboxsf
447dff7257fbc53f0b47ed873d2b02eb4773401c]
Jul 19 08:48:19 localhost kernel: ? __pfx_vboxsf_fill_super+0x10/0x10 [vboxsf
447dff7257fbc53f0b47ed873d2b02eb4773401c]
Jul 19 08:48:19 localhost kernel: vfs_get_super+0x6e/0xe0
Jul 19 08:48:19 localhost kernel: vfs_get_tree+0x29/0xd0
Jul 19 08:48:19 localhost kernel: path_mount+0x491/0xac0
Jul 19 08:48:19 localhost kernel: __x64_sys_mount+0x109/0x140
Jul 19 08:48:19 localhost kernel: do_syscall_64+0x60/0x90
Jul 19 08:48:19 localhost kernel: ? count_memcg_events.constprop.0+0x1a/0x30
Jul 19 08:48:19 localhost kernel: ? handle_mm_fault+0x9e/0x350
Jul 19 08:48:19 localhost kernel: ? do_user_addr_fault+0x225/0x640
Jul 19 08:48:19 localhost kernel: ? exc_page_fault+0x71/0x160
Jul 19 08:48:19 localhost kernel: entry_SYSCALL_64_after_hwframe+0x6e/0xd8
Jul 19 08:48:19 localhost kernel: RIP: 0033:0x7f22f0e05b9e
Jul 19 08:48:19 localhost kernel: Code: c3 66 2e 0f 1f 84 00 00 00 00 00 90 90
90 90 90 90 90 90 90 90 90 90 90 90 90 f3 0f 1e fa 66 90 49 89 ca b8 a5 00 00 00
0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 3a 02 0d 00 f7 d8 64 89 01 48
Jul 19 08:48:19 localhost kernel: RSP: 002b:00007ffe8ba4bab8 EFLAGS: 00000212
ORIG_RAX: 00000000000000a5
Jul 19 08:48:19 localhost kernel: RAX: ffffffffffffffda RBX: 00007ffe8ba4df04
RCX: 00007f22f0e05b9e
Jul 19 08:48:19 localhost kernel: RDX: 000055e9ab199ba8 RSI: 00007ffe8ba4df0b
RDI: 00007ffe8ba4df04
Jul 19 08:48:19 localhost kernel: RBP: 00007ffe8ba4bc00 R08: 00007ffe8ba4bf50
R09: 0000000000000000
Jul 19 08:48:19 localhost kernel: R10: 0000000000000004 R11: 0000000000000212
R12: 00007ffe8ba4df0b
Jul 19 08:48:19 localhost kernel: R13: 0000000000000011 R14: 00007ffe8ba4bf50
R15: 00007ffe8ba4df29
Jul 19 08:48:19 localhost kernel: </TASK>
Jul 19 08:48:19 localhost kernel: Modules linked in: vboxsf vboxguest fuse
efi_pstore configfs dmi_sysfs ip_tables x_tables crct10dif_pclmul crc32_pclmul
polyval_clmulni polyval_generic sr_mod ohci_pci gf128mul cdrom ohci_hcd ehci_pci
ehci_hcd ghash_clmulni_intel sha512_ssse3 ata_generic vmwgfx video usbcore
drm_ttm_helper ttm aesni_intel crypto_simd cryptd ata_piix wmi serio_raw btrfs
blake2b_generic libcrc32c crc32c_intel xor raid6_pq sg dm_multipath dm_mod
scsi_dh_rdac scsi_dh_emc scsi_dh_alua msr efivarfs
Jul 19 08:48:19 localhost kernel: ---[ end trace 0000000000000000 ]---
The traceback points to the strscpy() added in commit 883f8fe87686d, which
ironically was submitted to avoid buffer overflows using strlcpy(); however, I
do not think that is the problem. My suspicion is that it comes from struct
shfl_string, and the definition of the variable-length arrays in the union, and
that their lengths are confusing the kernel's string handling routines.
I will be happy to test any proposed patches.
Larry
On Wed, Jul 19, 2023 at 10:02:36AM -0500, Larry Finger wrote:
> Hi,
>
> When I try to start a VirtualBox virtual machine running kernel 6.5-rc2, it
> gets a kernel bug as follows while trying to mount a vboxsf-shared mount:
>
> Jul 19 08:48:19 localhost kernel: detected buffer overflow in strscpy
> Jul 19 08:48:19 localhost kernel: ------------[ cut here ]------------
> Jul 19 08:48:19 localhost kernel: kernel BUG at lib/string_helpers.c:1031!
> [...]
> Jul 19 08:48:19 localhost kernel: Call Trace:
> [...]
> Jul 19 08:48:19 localhost kernel: vboxsf_fill_super+0x3bc/0x3c0 [vboxsf 447dff7257fbc53f0b47ed873d2b02eb4773401c]
> [...]
>
> The traceback points to the strscpy() added in commit 883f8fe87686d, which
> ironically was submitted to avoid buffer overflows using strlcpy(); however,
> I do not think that is the problem. My suspicion is that it comes from
> struct shfl_string, and the definition of the variable-length arrays in the
> union, and that their lengths are confusing the kernel's string handling
> routines.
Ah, hm, I think this may still warn with 883f8fe87686d reverted, as it
seems the issue is the fake flexible arrays in struct shfl_string. Likely
the patch manifesting the false positive is df8fc4e934c1 ("kbuild:
Enable -fstrict-flex-arrays=3"), if you're building with GCC 13.
> I will be happy to test any proposed patches.
Thank! Can you see if this fixes it?
diff --git a/fs/vboxsf/shfl_hostintf.h b/fs/vboxsf/shfl_hostintf.h
index aca829062c12..243d1b91bb45 100644
--- a/fs/vboxsf/shfl_hostintf.h
+++ b/fs/vboxsf/shfl_hostintf.h
@@ -68,9 +68,8 @@ struct shfl_string {
/** UTF-8 or UTF-16 string. Nul terminated. */
union {
- u8 utf8[2];
- u16 utf16[1];
- u16 ucs2[1]; /* misnomer, use utf16. */
+ DECLARE_FLEX_ARRAY(u8, utf8);
+ DECLARE_FLEX_ARRAY(u16, utf16);
} string;
};
VMMDEV_ASSERT_SIZE(shfl_string, 6);
(I note that "ucs" is used in the kernel source, and contains a comment
that it shouldn't be used, so I removed it.)
--
Kees Cook
Okay, please try:
diff --git a/fs/vboxsf/shfl_hostintf.h b/fs/vboxsf/shfl_hostintf.h
index aca829062c12..902fe3224453 100644
--- a/fs/vboxsf/shfl_hostintf.h
+++ b/fs/vboxsf/shfl_hostintf.h
@@ -68,12 +68,11 @@ struct shfl_string {
/** UTF-8 or UTF-16 string. Nul terminated. */
union {
- u8 utf8[2];
- u16 utf16[1];
- u16 ucs2[1]; /* misnomer, use utf16. */
+ DECLARE_FLEX_ARRAY(u8, utf8);
+ DECLARE_FLEX_ARRAY(u16, utf16);
} string;
};
-VMMDEV_ASSERT_SIZE(shfl_string, 6);
+VMMDEV_ASSERT_SIZE(shfl_string, 4);
/* The size of shfl_string w/o the string part. */
#define SHFLSTRING_HEADER_SIZE 4
The size assert doesn't seem to be used anywhere else, but I can do a
more careful binary analysis later today...
--
Kees Cook
On Wed, Jul 19, 2023 at 08:31:08AM -0700, Kees Cook wrote:
> diff --git a/fs/vboxsf/shfl_hostintf.h b/fs/vboxsf/shfl_hostintf.h
> index aca829062c12..243d1b91bb45 100644
> --- a/fs/vboxsf/shfl_hostintf.h
> +++ b/fs/vboxsf/shfl_hostintf.h
> @@ -68,9 +68,8 @@ struct shfl_string {
>
> /** UTF-8 or UTF-16 string. Nul terminated. */
> union {
> - u8 utf8[2];
> - u16 utf16[1];
> - u16 ucs2[1]; /* misnomer, use utf16. */
> + DECLARE_FLEX_ARRAY(u8, utf8);
> + DECLARE_FLEX_ARRAY(u16, utf16);
> } string;
> };
> VMMDEV_ASSERT_SIZE(shfl_string, 6);
Oops, this doesn't even compile. Let me get that fixed...
--
Kees Cook
On 7/19/23 10:36, Kees Cook wrote:
> Okay, please try:
>
> diff --git a/fs/vboxsf/shfl_hostintf.h b/fs/vboxsf/shfl_hostintf.h
> index aca829062c12..902fe3224453 100644
> --- a/fs/vboxsf/shfl_hostintf.h
> +++ b/fs/vboxsf/shfl_hostintf.h
> @@ -68,12 +68,11 @@ struct shfl_string {
>
> /** UTF-8 or UTF-16 string. Nul terminated. */
> union {
> - u8 utf8[2];
> - u16 utf16[1];
> - u16 ucs2[1]; /* misnomer, use utf16. */
> + DECLARE_FLEX_ARRAY(u8, utf8);
> + DECLARE_FLEX_ARRAY(u16, utf16);
> } string;
> };
> -VMMDEV_ASSERT_SIZE(shfl_string, 6);
> +VMMDEV_ASSERT_SIZE(shfl_string, 4);
>
> /* The size of shfl_string w/o the string part. */
> #define SHFLSTRING_HEADER_SIZE 4
>
>
> The size assert doesn't seem to be used anywhere else, but I can do a
> more careful binary analysis later today...''
Kees,
The testing was harder than I expected. My standard kernel would not load the
system disk on the VM, thus I had to build one using my distros configuration.
It is really painful to wait for all those drivers to build, but I figured that
might be faster than trying to find the incorrect parameter.
I can finally report that vboxsf no longer generated a BUG. It is getting
farther, but I am not done yet. It now generates a line that says "Unknown
parameter tag" and then hangs. I have not tracked that down yet.
I was not aware of the DECLARE_FLEX_ARRAY() macro. I had considered removing the
union and creating a simple string[] declaration, but I do not mess with file
systems, and decided to let the experts handle it.
I will let you know what I find about that unknown parameter. It probably is
coming from VirtualBox.
Thanks,
Larry
On July 19, 2023 2:24:52 PM PDT, Larry Finger <[email protected]> wrote:
>On 7/19/23 10:36, Kees Cook wrote:
>> Okay, please try:
>>
>> diff --git a/fs/vboxsf/shfl_hostintf.h b/fs/vboxsf/shfl_hostintf.h
>> index aca829062c12..902fe3224453 100644
>> --- a/fs/vboxsf/shfl_hostintf.h
>> +++ b/fs/vboxsf/shfl_hostintf.h
>> @@ -68,12 +68,11 @@ struct shfl_string {
>> /** UTF-8 or UTF-16 string. Nul terminated. */
>> union {
>> - u8 utf8[2];
>> - u16 utf16[1];
>> - u16 ucs2[1]; /* misnomer, use utf16. */
>> + DECLARE_FLEX_ARRAY(u8, utf8);
>> + DECLARE_FLEX_ARRAY(u16, utf16);
>> } string;
>> };
>> -VMMDEV_ASSERT_SIZE(shfl_string, 6);
>> +VMMDEV_ASSERT_SIZE(shfl_string, 4);
>> /* The size of shfl_string w/o the string part. */
>> #define SHFLSTRING_HEADER_SIZE 4
>>
>>
>> The size assert doesn't seem to be used anywhere else, but I can do a
>> more careful binary analysis later today...''
>
>Kees,
>
>The testing was harder than I expected. My standard kernel would not load the system disk on the VM, thus I had to build one using my distros configuration. It is really painful to wait for all those drivers to build, but I figured that might be faster than trying to find the incorrect parameter.
>
>I can finally report that vboxsf no longer generated a BUG. It is getting farther, but I am not done yet. It now generates a line that says "Unknown parameter tag" and then hangs. I have not tracked that down yet.
>
>I was not aware of the DECLARE_FLEX_ARRAY() macro. I had considered removing the union and creating a simple string[] declaration, but I do not mess with file systems, and decided to let the experts handle it.
>
>I will let you know what I find about that unknown parameter. It probably is coming from VirtualBox.
It's possible the size really needs to stay 6 bytes. In that case, try adding a "u8 legacy_padding[2]" to the union and restore the VMMDEV_ASSERT_SIZE?
--
Kees Cook
On 7/19/23 17:37, Kees Cook wrote:
> On July 19, 2023 2:24:52 PM PDT, Larry Finger <[email protected]> wrote:
>> On 7/19/23 10:36, Kees Cook wrote:
>>> Okay, please try:
>>>
>>> diff --git a/fs/vboxsf/shfl_hostintf.h b/fs/vboxsf/shfl_hostintf.h
>>> index aca829062c12..902fe3224453 100644
>>> --- a/fs/vboxsf/shfl_hostintf.h
>>> +++ b/fs/vboxsf/shfl_hostintf.h
>>> @@ -68,12 +68,11 @@ struct shfl_string {
>>> /** UTF-8 or UTF-16 string. Nul terminated. */
>>> union {
>>> - u8 utf8[2];
>>> - u16 utf16[1];
>>> - u16 ucs2[1]; /* misnomer, use utf16. */
>>> + DECLARE_FLEX_ARRAY(u8, utf8);
>>> + DECLARE_FLEX_ARRAY(u16, utf16);
>>> } string;
>>> };
>>> -VMMDEV_ASSERT_SIZE(shfl_string, 6);
>>> +VMMDEV_ASSERT_SIZE(shfl_string, 4);
>>> /* The size of shfl_string w/o the string part. */
>>> #define SHFLSTRING_HEADER_SIZE 4
>>>
>>>
>>> The size assert doesn't seem to be used anywhere else, but I can do a
>>> more careful binary analysis later today...''
>>
>> Kees,
>>
>> The testing was harder than I expected. My standard kernel would not load the system disk on the VM, thus I had to build one using my distros configuration. It is really painful to wait for all those drivers to build, but I figured that might be faster than trying to find the incorrect parameter.
>>
>> I can finally report that vboxsf no longer generated a BUG. It is getting farther, but I am not done yet. It now generates a line that says "Unknown parameter tag" and then hangs. I have not tracked that down yet.
>>
>> I was not aware of the DECLARE_FLEX_ARRAY() macro. I had considered removing the union and creating a simple string[] declaration, but I do not mess with file systems, and decided to let the experts handle it.
>>
>> I will let you know what I find about that unknown parameter. It probably is coming from VirtualBox.
>
> It's possible the size really needs to stay 6 bytes. In that case, try adding a "u8 legacy_padding[2]" to the union and restore the VMMDEV_ASSERT_SIZE?
Kees,
You win the prize. Adding the 2 bytes of padding restored operations.
Thanks,
Larry