Crash in VirtualBox virtual machines running kernel 6.5

Crash in VirtualBox virtual machines running kernel 6.5

[Larry Finger]

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

Re: Crash in VirtualBox virtual machines running kernel 6.5

[Kees Cook]

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

Re: Crash in VirtualBox virtual machines running kernel 6.5

[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

Re: Crash in VirtualBox virtual machines running kernel 6.5

[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

Re: Crash in VirtualBox virtual machines running kernel 6.5

[Larry Finger]

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

Re: Crash in VirtualBox virtual machines running kernel 6.5

[Kees Cook]

On July 19, 2023 2:24:52 PM PDT, Larry Finger <Larry.Finger@lwfinger.net> 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

Re: Crash in VirtualBox virtual machines running kernel 6.5

[Larry Finger]

On 7/19/23 17:37, Kees Cook wrote:
> On July 19, 2023 2:24:52 PM PDT, Larry Finger <Larry.Finger@lwfinger.net> 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