[GIT PULL] hardening fixes for v6.6-rc3

[GIT PULL] hardening fixes for v6.6-rc3

[Kees Cook]

Hi Linus,

Please pull these hardening fixes for v6.6-rc3. These have been in -next
for a week now.

Thanks!

-Kees

The following changes since commit 5f536ac6a5a7b67351e4e5ae4f9e1e57d31268e6:

  LoadPin: Annotate struct dm_verity_loadpin_trusted_root_digest with __counted_by (2023-08-25 16:07:30 -0700)

are available in the Git repository at:

  https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git tags/hardening-v6.6-rc3

for you to fetch changes up to 32a4ec211d4164e667d9d0b807fadf02053cd2e9:

  uapi: stddef.h: Fix __DECLARE_FLEX_ARRAY for C++ (2023-09-13 20:09:49 -0700)

----------------------------------------------------------------
hardening fixes for v6.6-rc3

- Fix UAPI stddef.h to avoid C++-ism (Alexey Dobriyan)

- Fix harmless UAPI stddef.h header guard endif (Alexey Dobriyan)

----------------------------------------------------------------
Alexey Dobriyan (2):
      uapi: stddef.h: Fix header guard location
      uapi: stddef.h: Fix __DECLARE_FLEX_ARRAY for C++

 include/uapi/linux/stddef.h | 7 +++++++
 1 file changed, 7 insertions(+)

-- 
Kees Cook

Re: [GIT PULL] hardening fixes for v6.6-rc3

[Linus Torvalds]

On Fri, 22 Sept 2023 at 09:59, Kees Cook <keescook@chromium.org> wrote:
>
> - Fix UAPI stddef.h to avoid C++-ism (Alexey Dobriyan)

Ugh. Did we really have to make two different versions of that define?

Ok, so C++ did something stupid wrt an empty struct. Fine.

But I think we could have still shared the same definition by just
using the same 'zero-sized array' trick, regardless of any 'empty
struct has a size in C++'.

IOW, wouldn't this just work universally, without any "two completely
different versions" hack?

#define __DECLARE_FLEX_ARRAY(TYPE, NAME)        \
        struct { \
                char __empty_ ## NAME[0]; \
                TYPE NAME[]; \
        }

I didn't test. I'm just hating on that '#ifdef __cplusplus'.

                   Linus

Re: [GIT PULL] hardening fixes for v6.6-rc3

[pr-tracker-bot]

The pull request you sent on Fri, 22 Sep 2023 09:59:01 -0700:

> https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git tags/hardening-v6.6-rc3

has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/d90b0276af8f25a0b8ae081a30d1b2a61263393b

Thank you!

-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/prtracker.html

Re: [GIT PULL] hardening fixes for v6.6-rc3

[Kees Cook]

On Fri, Sep 22, 2023 at 04:55:45PM -0700, Linus Torvalds wrote:
> On Fri, 22 Sept 2023 at 09:59, Kees Cook <keescook@chromium.org> wrote:
> >
> > - Fix UAPI stddef.h to avoid C++-ism (Alexey Dobriyan)
> 
> Ugh. Did we really have to make two different versions of that define?
> 
> Ok, so C++ did something stupid wrt an empty struct. Fine.
> 
> But I think we could have still shared the same definition by just
> using the same 'zero-sized array' trick, regardless of any 'empty
> struct has a size in C++'.
> 
> IOW, wouldn't this just work universally, without any "two completely
> different versions" hack?
> 
> #define __DECLARE_FLEX_ARRAY(TYPE, NAME)        \
>         struct { \
>                 char __empty_ ## NAME[0]; \
>                 TYPE NAME[]; \
>         }
> 
> I didn't test. I'm just hating on that '#ifdef __cplusplus'.

Yeah, I had same thought[1], but in the end I left it the way Alexey
suggested for one decent reason, and one weak reason:

1) As discovered[2] while porting this helper to ACPICA, using a flexible
   array in a struct like this does not fly with MSVC, so for MSVC
   ingesting UAPI, having the separate struct is likely more robust.

2) __cplusplus is relatively common in UAPI headers already:
   $ git grep __cplusplus -- include/uapi | wc -l
   58

-Kees

[1] https://lore.kernel.org/all/202309151208.C99747375@keescook/
[2] https://github.com/acpica/acpica/pull/837

-- 
Kees Cook

Re: [GIT PULL] hardening fixes for v6.6-rc3

[Alexey Dobriyan]

On Fri, Sep 22, 2023 at 04:55:45PM -0700, Linus Torvalds wrote:
> On Fri, 22 Sept 2023 at 09:59, Kees Cook <keescook@chromium.org> wrote:
> >
> > - Fix UAPI stddef.h to avoid C++-ism (Alexey Dobriyan)
> 
> Ugh. Did we really have to make two different versions of that define?
> 
> Ok, so C++ did something stupid wrt an empty struct. Fine.
> 
> But I think we could have still shared the same definition by just
> using the same 'zero-sized array' trick, regardless of any 'empty
> struct has a size in C++'.
> 
> IOW, wouldn't this just work universally, without any "two completely
> different versions" hack?
> 
> #define __DECLARE_FLEX_ARRAY(TYPE, NAME)        \
>         struct { \
>                 char __empty_ ## NAME[0]; \
>                 TYPE NAME[]; \
>         }

This doesn't work with g++ :-(

#undef __DECLARE_FLEX_ARRAY
#define __DECLARE_FLEX_ARRAY(TYPE, NAME)        \
	struct { \
		char __empty_ ## NAME[0]; \
		TYPE NAME[]; \
	}

struct S1 {
	__DECLARE_FLEX_ARRAY(int, x);
};

main.cc:79:35: error: flexible array member ‘S1::<unnamed struct>::x’ in an otherwise empty ‘struct S1’
   79 |         __DECLARE_FLEX_ARRAY(int, x);

Re: [GIT PULL] hardening fixes for v6.6-rc3

[Linus Torvalds]

On Sat, 23 Sept 2023 at 09:53, Alexey Dobriyan <adobriyan@gmail.com> wrote:
>
> This doesn't work with g++ :-(

Whee. So the compiler seems to literally test "is it at offset 0", and
refuses to do flex arrays there.

Oh well. So flex arrays are just not usable on C++, because the
language tries to "protect" us from outselves. What else is new. I
suspect it's the same broken reason that empty structs aren't
zero-sized - stop the user from being clever.

I had hoped that the C++ people had learnt from their mistakes, but no.

Happily we don't have to deal with that crud for kernel code. I don't
like the #ifdef, but if it's needed...

                 Linus

Re: [GIT PULL] hardening fixes for v6.6-rc3

[Linus Torvalds]

On Fri, 22 Sept 2023 at 20:49, Kees Cook <keescook@chromium.org> wrote:
>
> 2) __cplusplus is relatively common in UAPI headers already:
>    $ git grep __cplusplus -- include/uapi | wc -l
>    58

Look a bit closer.

Most of those - by far - is for the usual

  #if defined(__cplusplus)
  extern "C" {
  #endif

pattern. IOW, it's explicitly not different code, but telling the C++
compiler that "this is C code".

So this new #ifdef is an ugly new pattern of "do totally different
things for C++".

Apparently required, but very ugly nonetheless.

               Linus

Re: [GIT PULL] hardening fixes for v6.6-rc3

[Alexey Dobriyan]

On Sat, Sep 23, 2023 at 11:04:57AM -0700, Linus Torvalds wrote:
> On Fri, 22 Sept 2023 at 20:49, Kees Cook <keescook@chromium.org> wrote:
> >
> > 2) __cplusplus is relatively common in UAPI headers already:
> >    $ git grep __cplusplus -- include/uapi | wc -l
> >    58
> 
> Look a bit closer.
> 
> Most of those - by far - is for the usual
> 
>   #if defined(__cplusplus)
>   extern "C" {
>   #endif
> 
> pattern. IOW, it's explicitly not different code, but telling the C++
> compiler that "this is C code".
> 
> So this new #ifdef is an ugly new pattern of "do totally different
> things for C++".
> 
> Apparently required, but very ugly nonetheless.

Most of those in uapi/ are likely unnecessary: extern "C" means
"don't mangle", but kernel doesn't export functions to userspace
except vDSO so there is nothing to mangle in the first place.

Re: [GIT PULL] hardening fixes for v6.6-rc3

[Linus Torvalds]

On Sun, 24 Sept 2023 at 09:58, Alexey Dobriyan <adobriyan@gmail.com> wrote:
>
> Most of those in uapi/ are likely unnecessary: extern "C" means
> "don't mangle", but kernel doesn't export functions to userspace
> except vDSO so there is nothing to mangle in the first place.

I suspect a lot of it is "this got copied-and-pasted from a source
that used it".

And even if you don't export, you have to *match* the linkage in case
you have the same name.

So I suspect that if you have any kind of prototype sharing between
user space (that might use C++) and kernel space, and end up with the
same helper functions in both cases, and having some header sharing,
you end up with that pattern. And you do it just once, and then it
spreads by copy-and-paste.

And then about a third of the hits seem to be in tools, which is
literally user space and probably actually has C and C++ mixing.

Another third is the drm uapi files. I didn't even try to look at what
the cause there is. But presumably there are common names used in user
space vs kernel.

And then the last third is random.

We do have a few other uses of __cplusplus. Sometimes just "we have a
structure member name that the C++ people decided was a magic
keyword".

So it's not like this new pattern is *completely* new - we've had
random "people want to use this header with C++ compilers and that
causes random issues" before. The details are different, the cause is
similar.

                Linus