Re: git: 034c0856018c - main - modules: fix freebsd32_modstat on big endian platforms

On Fri, 7 Jul 2023 18:59:41 +0100
Jessica Clarke <jrtc27@freebsd.org> wrote:

> On 7 Jul 2023, at 05:24, Ka Ho Ng <khng@FreeBSD.org> wrote:
> > 
> > The branch main has been updated by khng:
> > 
> > URL:
> > https://cgit.FreeBSD.org/src/commit/?id=034c0856018ccf73c0f2d6140825f3edf43f47b2
> > 
> > commit 034c0856018ccf73c0f2d6140825f3edf43f47b2
> > Author:     Ka Ho Ng <khng@FreeBSD.org>
> > AuthorDate: 2023-07-07 04:21:01 +0000
> > Commit:     Ka Ho Ng <khng@FreeBSD.org>
> > CommitDate: 2023-07-07 04:22:59 +0000
> > 
> >    modules: fix freebsd32_modstat on big endian platforms
> > 
> >    The layout of modspecific_t on both little endian and big endian
> > are as follows:
> >    |0|1|2|3|4|5|6|7|
> >    +-------+-------+
> >    |uintval|       |
> >    +-------+-------+
> >    |ulongval       |
> >    +-------+-------+
> > 
> >    For the following code snippet:
> >            CP(mod->data, data32, longval);
> >            CP(mod->data, data32, ulongval);
> >    It only takes care of little endian platforms that it truncates
> > the highest 32bit automatically. However on big endian platforms it
> > takes the highest 32bit instead. This eventually returns a garbage
> > syscall number to the 32bit userland.  
> 
> This fixes the case where uintval is the active member, but breaks the
> case where ulongval is the active member, since it’ll take bytes 0, 1,
> 2 and 3 which are the high bytes for BE, not the low bytes. This is an
> intrinsic issue with BE; LE is very permissive when you access the
> wrong union member for different-width integer types, but BE is not
> and you have to access the right one. How does userspace know which
> member to access, anyway? The kernel needs to repeat that and copy
> the right member based on that information, otherwise it is
> inherently impossible to have both cases work on BE.
> 
> >    Since modspecific_t's usage currently is for the use of syscall
> > modules, we only initialize modspecific32_t with uintval.  
> 
> Is this saying that uintval is always the active member, never
> ulongval? If so, there need to be comments, both in freebsd32_modstat
> to justify this and on the type to say “DO NOT USE (u)longval”,
> otherwise there is a significant risk someone will start using
> (u)longval and we’ll be stuck. Perhaps (u)longval should even be
> renamed to be ABI-compatibility padding (and aligning).
> 
> Jess

A better solution is to make these fields fixed size, so we don't run
into this problem again.  Is this feasible, factoring in any other
consumers?

- Justin

> 
> >    Now on both BE and LE
> >    64-bit platforms it always pick up the first 4 bytes.
> > 
> >    Sponsored by:   Juniper Networks, Inc.
> >    Reviewed by:    markj
> >    Differential Revision:  https://reviews.freebsd.org/D40814
> >    MFC after:      1 week
> > ---
> > sys/kern/kern_module.c | 5 ++---
> > 1 file changed, 2 insertions(+), 3 deletions(-)
> > 
> > diff --git a/sys/kern/kern_module.c b/sys/kern/kern_module.c
> > index 40e47868481c..d4edc64a5ce4 100644
> > --- a/sys/kern/kern_module.c
> > +++ b/sys/kern/kern_module.c
> > @@ -520,10 +520,9 @@ freebsd32_modstat(struct thread *td, struct
> > freebsd32_modstat_args *uap) id = mod->id;
> > refs = mod->refs;
> > name = mod->name;
> > - CP(mod->data, data32, intval);
> > + _Static_assert(sizeof(data32) == sizeof(data32.uintval),
> > +    "bad modspecific32_t size");
> > CP(mod->data, data32, uintval);
> > - CP(mod->data, data32, longval);
> > - CP(mod->data, data32, ulongval);
> > MOD_SUNLOCK;
> > stat32 = uap->stat;
> >