diff mbox series

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

Message ID 10e97a04980d933b2cfecb6b124bf9046b6e4f16.1582216144.git.leonard.crestez@nxp.com
State Not Applicable
Headers show
Series firmware: imx: Align imx SC msg structs to 4 | expand

Commit Message

Leonard Crestez Feb. 20, 2020, 4:29 p.m. UTC
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(-)

Comments

Shawn Guo Feb. 24, 2020, 7:22 a.m. UTC | #1
On Thu, Feb 20, 2020 at 06:29:32PM +0200, Leonard Crestez wrote:
> 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>

Applied, thanks.
Stephen Boyd Feb. 25, 2020, 4:51 p.m. UTC | #2
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 see that the calling code puts 3 as the 'size' for this struct in
clk_scu_set_rate().

	hdr->size = 3;

That seems to say that the struct is 3 words long, or 12 bytes. Then we
call imx_scu_call_rpc(), passing the pointer to this struct on the stack
and that eventually gets into imx_scu_ipc_write() calling
mbox_send_message() with u32 pointers.

	for (i = 0; i < hdr->size; i++) {
		sc_chan = &sc_ipc->chans[i % 4];
		ret = mbox_send_message(sc_chan->ch, &data[i]);

So we've taken the 11 byte struct (data in this case) and casted it to a
u32 array with 3 elements, which is bad. This is what kasan is warning
about. Adding aligned sometimes fixes it because the compiler will place
the next stack variable at the naturally aligned location and thus we
get the one byte padding but I don't see how that works when it's the
last stack element. The stack will end at some unaligned address.

The better solution would be to drop __aligned(4) and make a union of
the struct with whatever size number of words the message is or do a
copy of the struct into a u32 array that is passed to
imx_scu_call_rpc().

For example:

	struct imx_sc_msg_req_set_clock_rate {
		union {
			struct packed_message {
				struct imx_sc_rpc_msg hdr;
				__le32 rate;
				__le16 resource;
				u8 clk;
			} __packed;
			u32 data[3];
		};
	};

If the union approach was used then each time imx_scu_call_rpc() is
called we can simply pass the 'data' member and make the second argument
'msg' strongly typed to be a u32 pointer. kasan should be happy too.
Leonard Crestez Feb. 25, 2020, 7:52 p.m. UTC | #3
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 see that the calling code puts 3 as the 'size' for this struct in
> clk_scu_set_rate().
> 
> 	hdr->size = 3;
> 
> That seems to say that the struct is 3 words long, or 12 bytes. Then we
> call imx_scu_call_rpc(), passing the pointer to this struct on the stack
> and that eventually gets into imx_scu_ipc_write() calling
> mbox_send_message() with u32 pointers.
> 
> 	for (i = 0; i < hdr->size; i++) {
> 		sc_chan = &sc_ipc->chans[i % 4];
> 		ret = mbox_send_message(sc_chan->ch, &data[i]);
> 
> So we've taken the 11 byte struct (data in this case) and casted it to a
> u32 array with 3 elements, which is bad. This is what kasan is warning
> about. Adding aligned sometimes fixes it because the compiler will place
> the next stack variable at the naturally aligned location and thus we
> get the one byte padding but I don't see how that works when it's the
> last stack element. The stack will end at some unaligned address.
> 
> The better solution would be to drop __aligned(4) and make a union of
> the struct with whatever size number of words the message is or do a
> copy of the struct into a u32 array that is passed to
> imx_scu_call_rpc().
> 
> For example:
> 
> 	struct imx_sc_msg_req_set_clock_rate {
> 		union {
> 			struct packed_message {
> 				struct imx_sc_rpc_msg hdr;
> 				__le32 rate;
> 				__le16 resource;
> 				u8 clk;
> 			} __packed;
> 			u32 data[3];
> 		};
> 	};
> 
> If the union approach was used then each time imx_scu_call_rpc() is
> called we can simply pass the 'data' member and make the second argument
> 'msg' strongly typed to be a u32 pointer. kasan should be happy too.
>
Shawn Guo March 16, 2020, 12:25 a.m. UTC | #4
On Mon, Feb 24, 2020 at 03:22:18PM +0800, Shawn Guo wrote:
> On Thu, Feb 20, 2020 at 06:29:32PM +0200, Leonard Crestez wrote:
> > 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>
> 
> Applied, thanks.

Patch #1 and #2 were dropped from my clk queue, as Stephen hasn't been
convinced by this change.

Shawn
Aisheng Dong March 16, 2020, 8:52 a.m. UTC | #5
> From: Stephen Boyd <sboyd@kernel.org>
> Sent: Wednesday, February 26, 2020 12:52 AM 
> 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.

Hi Leonard,

Can you construct this case to see if we can reproduce the issue as pointed by Stephen?

Regards
Aisheng

> 
> I see that the calling code puts 3 as the 'size' for this struct in clk_scu_set_rate().
> 
> 	hdr->size = 3;
> 
> That seems to say that the struct is 3 words long, or 12 bytes. Then we call
> imx_scu_call_rpc(), passing the pointer to this struct on the stack and that
> eventually gets into imx_scu_ipc_write() calling
> mbox_send_message() with u32 pointers.
> 
> 	for (i = 0; i < hdr->size; i++) {
> 		sc_chan = &sc_ipc->chans[i % 4];
> 		ret = mbox_send_message(sc_chan->ch, &data[i]);
> 
> So we've taken the 11 byte struct (data in this case) and casted it to a
> u32 array with 3 elements, which is bad. This is what kasan is warning about.
> Adding aligned sometimes fixes it because the compiler will place the next stack
> variable at the naturally aligned location and thus we get the one byte padding
> but I don't see how that works when it's the last stack element. The stack will
> end at some unaligned address.
> 
> The better solution would be to drop __aligned(4) and make a union of the
> struct with whatever size number of words the message is or do a copy of the
> struct into a u32 array that is passed to imx_scu_call_rpc().
> 
> For example:
> 
> 	struct imx_sc_msg_req_set_clock_rate {
> 		union {
> 			struct packed_message {
> 				struct imx_sc_rpc_msg hdr;
> 				__le32 rate;
> 				__le16 resource;
> 				u8 clk;
> 			} __packed;
> 			u32 data[3];
> 		};
> 	};
> 
> If the union approach was used then each time imx_scu_call_rpc() is called we
> can simply pass the 'data' member and make the second argument 'msg'
> strongly typed to be a u32 pointer. kasan should be happy too.
diff mbox series

Patch

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);
 
 struct req_get_clock_rate {
 	__le16 resource;
 	u8 clk;
-} __packed;
+} __packed __aligned(4);
 
 struct resp_get_clock_rate {
 	__le32 rate;
 };
 
@@ -119,11 +119,11 @@  struct imx_sc_msg_req_clock_enable {
 	struct imx_sc_rpc_msg hdr;
 	__le16 resource;
 	u8 clk;
 	u8 enable;
 	u8 autog;
-} __packed;
+} __packed __aligned(4);
 
 static inline struct clk_scu *to_clk_scu(struct clk_hw *hw)
 {
 	return container_of(hw, struct clk_scu, hw);
 }