Re: [PATCH v2 1/8] clk: imx: Align imx sc clock msg structs to 4

[Dmitry Vyukov <dvyukov@google.com>]

On Tue, Mar 17, 2020 at 8:25 PM Leonard Crestez <leonard.crestez@nxp.com> wrote:
>
> On 2020-02-27 3:48 AM, Stephen Boyd wrote:
> > Quoting Leonard Crestez (2020-02-25 11:52:11)
> >> On 25.02.2020 18:52, Stephen Boyd wrote:
> >>> Quoting Leonard Crestez (2020-02-20 08:29:32)
> >>>> The imx SC api strongly assumes that messages are composed out of
> >>>> 4-bytes words but some of our message structs have odd sizeofs.
> >>>>
> >>>> This produces many oopses with CONFIG_KASAN=y.
> >>>>
> >>>> Fix by marking with __aligned(4).
> >>>>
> >>>> Fixes: fe37b4820417 ("clk: imx: add scu clock common part")
> >>>> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
> >>>> ---
> >>>>    drivers/clk/imx/clk-scu.c | 6 +++---
> >>>>    1 file changed, 3 insertions(+), 3 deletions(-)
> >>>>
> >>>> diff --git a/drivers/clk/imx/clk-scu.c b/drivers/clk/imx/clk-scu.c
> >>>> index fbef740704d0..3c5c42d8833e 100644
> >>>> --- a/drivers/clk/imx/clk-scu.c
> >>>> +++ b/drivers/clk/imx/clk-scu.c
> >>>> @@ -41,16 +41,16 @@ struct clk_scu {
> >>>>    struct imx_sc_msg_req_set_clock_rate {
> >>>>           struct imx_sc_rpc_msg hdr;
> >>>>           __le32 rate;
> >>>>           __le16 resource;
> >>>>           u8 clk;
> >>>> -} __packed;
> >>>> +} __packed __aligned(4);
> >>>
> >>> Sorry, this still doesn't make sense to me. Having __aligned(4) means
> >>> that the struct is placed on the stack at some alignment, great, but it
> >>> still has __packed so the sizeof this struct is some odd number like 11.
> >>> If this struct is the last element on the stack it will end at some
> >>> unaligned address and the mailbox code will read a few bytes beyond the
> >>> end of the stack.
> >>
> >> I checked again and marking the struct with __aligned(4) makes it have
> >> sizeof == 12 as intended. It was 11 before.
> >>
> >>       static_assert(sizeof(struct imx_sc_msg_req_set_clock_rate) == 12);
> >>
> >> After reading through your email and gcc docs again I'm not sure if this
> >> portable/reliable this is but as far as I understand "sizeof" needs to
> >> account for alignment. Or is this just an accident with my compiler?
> >>
> >> Marking a structure both __packed and __aligned(4) means that __packed
> >> only affects internal struct member layout but sizeof is still rounded
> >> up to a multiple of 4:
> >>
> >> struct test {
> >>          u8      a;
> >>          u16     b;
> >> } __packed __aligned(4);
> >>
> >> static_assert(sizeof(struct test) == 4);
> >> static_assert(offsetof(struct test, a) == 0);
> >> static_assert(offsetof(struct test, b) == 1);
> >>
> >> This test is not realistic because I don't think SCU messages have any
> >> such oddly-aligned members.
> >>
> >
> > I'm not really sure as I'm not a linker expert. I'm just especially wary
> > of using __packed or __aligned attributes because they silently generate
> > code that is usually inefficient. This is why we typically do lots of
> > shifting and masking in the kernel, so that we can easily see how
> > complicated it is to pack bits into place. Maybe it makes sense to get
> > rid of the structs entirely and pack the bits into __le32 arrays of
> > varying length. Then we don't have to worry about packed or aligned or
> > what the compiler will do and we can easily be confident that we've put
> > the bits in the right place in each u32 that is eventually written to
> > the mailbox register space.
>
> These message structs are not as complicated as hardware register, for
> example everything is always on a byte border.
>
> In older versions of the imx internal tree SC messaging is done by
> packing into arrays through a layer of generated code which looks like this:
>
>           RPC_VER(&msg) = SC_RPC_VERSION;
>           RPC_SVC(&msg) = U8(SC_RPC_SVC_MISC);
>           RPC_FUNC(&msg) = U8(MISC_FUNC_SET_CONTROL);
>           RPC_U32(&msg, 0U) = U32(ctrl);
>           RPC_U32(&msg, 4U) = U32(val);
>           RPC_U16(&msg, 8U) = U16(resource);
>           RPC_SIZE(&msg) = 4U;
>
> The RPC_U32/U16 macros look like this:
>
> #define RPC_I32(MESG, IDX)      ((MESG)->DATA.i32[(IDX) / 4U])
> #define RPC_I16(MESG, IDX)      ((MESG)->DATA.i16[(IDX) / 2U])
> #define RPC_I8(MESG, IDX)       ((MESG)->DATA.i8[(IDX)])
> #define RPC_U32(MESG, IDX)      ((MESG)->DATA.u32[(IDX) / 4U])
> #define RPC_U16(MESG, IDX)      ((MESG)->DATA.u16[(IDX) / 2U])
> #define RPC_U8(MESG, IDX)       ((MESG)->DATA.u8[(IDX)])
>
> and the message struct itself has a big union for the data:
>
> typedef struct {
>           uint8_t version;
>           uint8_t size;
>           uint8_t svc;
>           uint8_t func;
>           union {
>                   int32_t i32[(SC_RPC_MAX_MSG - 1U)];
>                   int16_t i16[(SC_RPC_MAX_MSG - 1U) * 2U];
>                   int8_t i8[(SC_RPC_MAX_MSG - 1U) * 4U];
>                   uint32_t u32[(SC_RPC_MAX_MSG - 1U)];
>                   uint16_t u16[(SC_RPC_MAX_MSG - 1U) * 2U];
>                   uint8_t u8[(SC_RPC_MAX_MSG - 1U) * 4U];
>           } DATA;
> } sc_rpc_msg_t;
>
> This approach is very verbose to the point of being unreadable I think
> it's much to message structs instead. Compiler struct layout rules are
> not really all that complicated and casting binary data as structs is
> very common in areas such as networking. This approach is also used by
> other firmware interfaces like TI sci and nvidia bpmp.
>
> imx8 currently has manually written message structs, it's unfortunate
> that a bug was found and fixing required a scattering patches in
> multiple subsystems. Perhaps a better solution would be to centralize
> all structs in a single header similar to drivers/firmware/ti_sci.h?
>
> In order to ensrue that there are no issues specific to the compile
> version perhaps a bunch of static_assert statements could be added to
> check that sizeof and offset are as expected?
>
> ---------------------------------
>
> As far as I can tell the issue KASAN warns about can be simplified to this:
>
> struct __packed badpack {
>      u32     a;
>      u16     b;
>      u8      c;
> };
>
> static_assert(sizeof(struct badpack) == 7);
>
> static void func(void *x)
> {
>      u32* arr = (u32*)x;
>      arr[0] = 0x11111111;
>      arr[1] = 0x22222222;
> }
>
> static int hello(void)
> {
>      struct badpack s;
>      u8 x = 0x33;
>
>      printk("&s=%px &x=%px\n", &s, &x);
>      func(&s);
>      // x could be overwritten here, depending on stack layout.
>      BUG_ON(x != 0x33);
>
>      return 0;
> }
>
> Adding __aligned(4) bumps struct size to 8 and avoids the issue
>
> Added KASAN maintainers to check if this is a valid fix.

Hi Leonard,

I think it should fix the bug.
It's not so much about KASAN, more about the validity of the C program.