* Re: Crash in VirtualBox virtual machines running kernel 6.5
[not found] <ab3a70e9-60ed-0f13-e3d4-8866eaccc8c1@lwfinger.net>
@ 2023-07-19 15:31 ` Kees Cook
2023-07-19 15:34 ` Kees Cook
2023-07-19 15:36 ` Kees Cook
0 siblings, 2 replies; 6+ messages in thread
From: Kees Cook @ 2023-07-19 15:31 UTC (permalink / raw)
To: Larry Finger
Cc: Hans de Goede, Azeem Shaikh, LKML, linux-hardening,
Gustavo A. R. Silva
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: Crash in VirtualBox virtual machines running kernel 6.5
2023-07-19 15:31 ` Crash in VirtualBox virtual machines running kernel 6.5 Kees Cook
@ 2023-07-19 15:34 ` Kees Cook
2023-07-19 15:36 ` Kees Cook
1 sibling, 0 replies; 6+ messages in thread
From: Kees Cook @ 2023-07-19 15:34 UTC (permalink / raw)
To: Larry Finger
Cc: Hans de Goede, Azeem Shaikh, LKML, linux-hardening,
Gustavo A. R. Silva
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Crash in VirtualBox virtual machines running kernel 6.5
2023-07-19 15:31 ` Crash in VirtualBox virtual machines running kernel 6.5 Kees Cook
2023-07-19 15:34 ` Kees Cook
@ 2023-07-19 15:36 ` Kees Cook
2023-07-19 21:24 ` Larry Finger
1 sibling, 1 reply; 6+ messages in thread
From: Kees Cook @ 2023-07-19 15:36 UTC (permalink / raw)
To: Larry Finger
Cc: Hans de Goede, Azeem Shaikh, LKML, linux-hardening,
Gustavo A. R. Silva
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: Crash in VirtualBox virtual machines running kernel 6.5
2023-07-19 15:36 ` Kees Cook
@ 2023-07-19 21:24 ` Larry Finger
2023-07-19 22:37 ` Kees Cook
0 siblings, 1 reply; 6+ messages in thread
From: Larry Finger @ 2023-07-19 21:24 UTC (permalink / raw)
To: Kees Cook
Cc: Hans de Goede, Azeem Shaikh, LKML, linux-hardening,
Gustavo A. R. Silva
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Crash in VirtualBox virtual machines running kernel 6.5
2023-07-19 21:24 ` Larry Finger
@ 2023-07-19 22:37 ` Kees Cook
2023-07-20 14:51 ` Larry Finger
0 siblings, 1 reply; 6+ messages in thread
From: Kees Cook @ 2023-07-19 22:37 UTC (permalink / raw)
To: Larry Finger, Kees Cook
Cc: Hans de Goede, Azeem Shaikh, LKML, linux-hardening,
Gustavo A. R. Silva
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Crash in VirtualBox virtual machines running kernel 6.5
2023-07-19 22:37 ` Kees Cook
@ 2023-07-20 14:51 ` Larry Finger
0 siblings, 0 replies; 6+ messages in thread
From: Larry Finger @ 2023-07-20 14:51 UTC (permalink / raw)
To: Kees Cook, Kees Cook
Cc: Hans de Goede, Azeem Shaikh, LKML, linux-hardening,
Gustavo A. R. Silva
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-07-20 14:51 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <ab3a70e9-60ed-0f13-e3d4-8866eaccc8c1@lwfinger.net>
2023-07-19 15:31 ` Crash in VirtualBox virtual machines running kernel 6.5 Kees Cook
2023-07-19 15:34 ` Kees Cook
2023-07-19 15:36 ` Kees Cook
2023-07-19 21:24 ` Larry Finger
2023-07-19 22:37 ` Kees Cook
2023-07-20 14:51 ` Larry Finger
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox