* 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