Re: [PATCH] ALSA: compress: Fix the mismatch size of struc between share lib(32bit) and kernel(64bit)

Re: [PATCH] ALSA: compress: Fix the mismatch size of struc between share lib(32bit) and kernel(64bit)

[Takashi Iwai]

At Mon, 09 Jun 2014 16:46:32 -0400,
Wang, Xiaoming wrote:
> 
> 
> The size of struct snd_compr_avail is 0x1c in 32bit kernel,
> while it is 0x20 in 64bit kernel 0x4 bytes added because of
> alignment. It is OK when 32bit kernel met 32bit user space.
> There exist stack corruption if 64bit kernel met 32bit user
> space, because the size of struct snd_compr_avail is 0x1c
> in 32bit user space which is smaller than it will get from
> kernel. The extra 4 bytes can corrupt the stack, and
> introduce unpredictable error.
> 
> Signed-off-by: Zhang Dongxing <dongxing.zhang@intel.com>
> Signed-off-by: xiaoming wang <xiaoming.wang@intel.com>

This would break the existing 32bit systems, so I don't think we can
take this approach.

Either break the 64bit systems (which aren't deployed yet much, so
far) by adding packed attribute, or implement 32/64 bit conversion in
compat_ioctl fop.


thanks,

Takashi

> ---
>  include/uapi/sound/compress_offload.h |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/include/uapi/sound/compress_offload.h b/include/uapi/sound/compress_offload.h
> index 5759810..766b416 100644
> --- a/include/uapi/sound/compress_offload.h
> +++ b/include/uapi/sound/compress_offload.h
> @@ -70,6 +70,7 @@ struct snd_compr_tstamp {
>  	__u32 pcm_frames;
>  	__u32 pcm_io_frames;
>  	__u32 sampling_rate;
> +	__u32 reserved[1];
>  };
>  
>  /**
> -- 
> 1.7.1
> 

Re: [PATCH] ALSA: compress: Fix the mismatch size of struc between share lib(32bit) and kernel(64bit)

[Vinod Koul]

On Mon, Jun 09, 2014 at 09:24:53AM +0200, Takashi Iwai wrote:
> At Mon, 09 Jun 2014 16:46:32 -0400,
> Wang, Xiaoming wrote:
> > 
> > 
> > The size of struct snd_compr_avail is 0x1c in 32bit kernel,
> > while it is 0x20 in 64bit kernel 0x4 bytes added because of
> > alignment. It is OK when 32bit kernel met 32bit user space.
> > There exist stack corruption if 64bit kernel met 32bit user
> > space, because the size of struct snd_compr_avail is 0x1c
> > in 32bit user space which is smaller than it will get from
> > kernel. The extra 4 bytes can corrupt the stack, and
> > introduce unpredictable error.
> > 
> > Signed-off-by: Zhang Dongxing <dongxing.zhang@intel.com>
> > Signed-off-by: xiaoming wang <xiaoming.wang@intel.com>
> 
> This would break the existing 32bit systems, so I don't think we can
> take this approach.
> 
> Either break the 64bit systems (which aren't deployed yet much, so
> far) by adding packed attribute, or implement 32/64 bit conversion in
> compat_ioctl fop.
I think former should be safe for now. Anyway we have only 1 driver using this
in mainline so fallout shouldn't be widespread!

-- 
~Vinod
> 
> 
> thanks,
> 
> Takashi
> 
> > ---
> >  include/uapi/sound/compress_offload.h |    1 +
> >  1 files changed, 1 insertions(+), 0 deletions(-)
> > 
> > diff --git a/include/uapi/sound/compress_offload.h b/include/uapi/sound/compress_offload.h
> > index 5759810..766b416 100644
> > --- a/include/uapi/sound/compress_offload.h
> > +++ b/include/uapi/sound/compress_offload.h
> > @@ -70,6 +70,7 @@ struct snd_compr_tstamp {
> >  	__u32 pcm_frames;
> >  	__u32 pcm_io_frames;
> >  	__u32 sampling_rate;
> > +	__u32 reserved[1];
> >  };
> >  
> >  /**
> > -- 
> > 1.7.1
> > 

-- 

[PATCH] ALSA: compress: Fix the mismatch size of struc between share lib(32bit) and kernel(64bit)

[Wang, Xiaoming]

The size of struct snd_compr_avail is 0x1c in 32bit kernel,
while it is 0x20 in 64bit kernel 0x4 bytes added because of
alignment. It is OK when 32bit kernel met 32bit user space.
There exist stack corruption if 64bit kernel met 32bit user
space, because the size of struct snd_compr_avail is 0x1c
in 32bit user space which is smaller than it will get from
kernel. The extra 4 bytes can corrupt the stack, and
introduce unpredictable error.

Signed-off-by: Zhang Dongxing <dongxing.zhang@intel.com>
Signed-off-by: xiaoming wang <xiaoming.wang@intel.com>
---
 include/uapi/sound/compress_offload.h |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/include/uapi/sound/compress_offload.h b/include/uapi/sound/compress_offload.h
index 5759810..766b416 100644
--- a/include/uapi/sound/compress_offload.h
+++ b/include/uapi/sound/compress_offload.h
@@ -70,6 +70,7 @@ struct snd_compr_tstamp {
 	__u32 pcm_frames;
 	__u32 pcm_io_frames;
 	__u32 sampling_rate;
+	__u32 reserved[1];
 };
 
 /**
-- 
1.7.1

RE: [PATCH] ALSA: compress: Fix the mismatch size of struc between share lib(32bit) and kernel(64bit)

[Wang, Xiaoming]

Dear Takashi

> -----Original Message-----
> From: Koul, Vinod
> Sent: Monday, June 09, 2014 11:36 PM
> To: Takashi Iwai
> Cc: Wang, Xiaoming; Kp, Jeeja; dhowells@redhat.com; arnd@arndb.de;
> tglx@linutronix.de; mtk.manpages@gmail.com;
> paulmck@linux.vnet.ibm.com; davej@redhat.com;
> linux-kernel@vger.kernel.org; Zhang, Dongxing
> Subject: Re: [PATCH] ALSA: compress: Fix the mismatch size of struc between
> share lib(32bit) and kernel(64bit)
> 
> On Mon, Jun 09, 2014 at 09:24:53AM +0200, Takashi Iwai wrote:
> > At Mon, 09 Jun 2014 16:46:32 -0400,
> > Wang, Xiaoming wrote:
> > >
> > >
> > > The size of struct snd_compr_avail is 0x1c in 32bit kernel, while it
> > > is 0x20 in 64bit kernel 0x4 bytes added because of alignment. It is
> > > OK when 32bit kernel met 32bit user space.
> > > There exist stack corruption if 64bit kernel met 32bit user space,
> > > because the size of struct snd_compr_avail is 0x1c in 32bit user
> > > space which is smaller than it will get from kernel. The extra 4
> > > bytes can corrupt the stack, and introduce unpredictable error.
> > >
> > > Signed-off-by: Zhang Dongxing <dongxing.zhang@intel.com>
> > > Signed-off-by: xiaoming wang <xiaoming.wang@intel.com>
> >
> > This would break the existing 32bit systems, so I don't think we can
> > take this approach.
> >
> > Either break the 64bit systems (which aren't deployed yet much, so
> > far) by adding packed attribute, or implement 32/64 bit conversion in
> > compat_ioctl fop.
> I think former should be safe for now. Anyway we have only 1 driver using
> this in mainline so fallout shouldn't be widespread!
Can you accept we add __attribute__((packed, aligned(4))) for struct snd_compr_avail
Which include the struct snd_compr_tstamp and fix the sizeof struct snd_compr_avail
to 0x1c both in 32bit and 64 bit kernel?
> 
> --
> ~Vinod
> >
> >
> > thanks,
> >
> > Takashi
> >
> > > ---
> > >  include/uapi/sound/compress_offload.h |    1 +
> > >  1 files changed, 1 insertions(+), 0 deletions(-)
> > >
> > > diff --git a/include/uapi/sound/compress_offload.h
> > > b/include/uapi/sound/compress_offload.h
> > > index 5759810..766b416 100644
> > > --- a/include/uapi/sound/compress_offload.h
> > > +++ b/include/uapi/sound/compress_offload.h
> > > @@ -70,6 +70,7 @@ struct snd_compr_tstamp {
> > >  	__u32 pcm_frames;
> > >  	__u32 pcm_io_frames;
> > >  	__u32 sampling_rate;
> > > +	__u32 reserved[1];
> > >  };
> > >
> > >  /**
> > > --
> > > 1.7.1
> > >
> 
> --

Re: [PATCH] ALSA: compress: Fix the mismatch size of struc between share lib(32bit) and kernel(64bit)

[Arnd Bergmann]

On Monday 09 June 2014, Vinod Koul wrote:
> On Mon, Jun 09, 2014 at 09:24:53AM +0200, Takashi Iwai wrote:
> > At Mon, 09 Jun 2014 16:46:32 -0400,
> > Wang, Xiaoming wrote:
> > > 
> > > 
> > > The size of struct snd_compr_avail is 0x1c in 32bit kernel,
> > > while it is 0x20 in 64bit kernel 0x4 bytes added because of
> > > alignment. It is OK when 32bit kernel met 32bit user space.
> > > There exist stack corruption if 64bit kernel met 32bit user
> > > space, because the size of struct snd_compr_avail is 0x1c
> > > in 32bit user space which is smaller than it will get from
> > > kernel. The extra 4 bytes can corrupt the stack, and
> > > introduce unpredictable error.
> > > 
> > > Signed-off-by: Zhang Dongxing <dongxing.zhang@intel.com>
> > > Signed-off-by: xiaoming wang <xiaoming.wang@intel.com>
> > 
> > This would break the existing 32bit systems, so I don't think we can
> > take this approach.
> > 
> > Either break the 64bit systems (which aren't deployed yet much, so
> > far) by adding packed attribute, or implement 32/64 bit conversion in
> > compat_ioctl fop.
> I think former should be safe for now. Anyway we have only 1 driver using this
> in mainline so fallout shouldn't be widespread!

I guess since the driver was only merged for 3.16, we don't
really have to worry about the ABI breakage anyway, but you can
still use that approach if you have reason to stay compatible with
existing used space built against out-of-tree drivers.

Anyway, if you use the __packed attribute, best apply it only to
the individual __u64 member(s), not the entire struct, otherwise
you might change user space programs in a subtle way when the alignment
changes from 4 to 1 byte. 

	Arnd

Re: [PATCH] ALSA: compress: Fix the mismatch size of struc between share lib(32bit) and kernel(64bit)

[Vinod Koul]

On Fri, Jun 13, 2014 at 04:19:32PM +0200, Arnd Bergmann wrote:
> On Monday 09 June 2014, Vinod Koul wrote:
> > On Mon, Jun 09, 2014 at 09:24:53AM +0200, Takashi Iwai wrote:
> > > At Mon, 09 Jun 2014 16:46:32 -0400,
> > > Wang, Xiaoming wrote:
> > > > 
> > > > 
> > > > The size of struct snd_compr_avail is 0x1c in 32bit kernel,
> > > > while it is 0x20 in 64bit kernel 0x4 bytes added because of
> > > > alignment. It is OK when 32bit kernel met 32bit user space.
> > > > There exist stack corruption if 64bit kernel met 32bit user
> > > > space, because the size of struct snd_compr_avail is 0x1c
> > > > in 32bit user space which is smaller than it will get from
> > > > kernel. The extra 4 bytes can corrupt the stack, and
> > > > introduce unpredictable error.
> > > > 
> > > > Signed-off-by: Zhang Dongxing <dongxing.zhang@intel.com>
> > > > Signed-off-by: xiaoming wang <xiaoming.wang@intel.com>
> > > 
> > > This would break the existing 32bit systems, so I don't think we can
> > > take this approach.
> > > 
> > > Either break the 64bit systems (which aren't deployed yet much, so
> > > far) by adding packed attribute, or implement 32/64 bit conversion in
> > > compat_ioctl fop.
> > I think former should be safe for now. Anyway we have only 1 driver using this
> > in mainline so fallout shouldn't be widespread!
> 
> I guess since the driver was only merged for 3.16, we don't
> really have to worry about the ABI breakage anyway, but you can
> still use that approach if you have reason to stay compatible with
> existing used space built against out-of-tree drivers.
The qcom has on out of tree driver, but not sure if they have used on 64bit so
they might not be impacted.

> Anyway, if you use the __packed attribute, best apply it only to
> the individual __u64 member(s), not the entire struct, otherwise
> you might change user space programs in a subtle way when the alignment
> changes from 4 to 1 byte. 
then wouldn't it make sense to call out the aligned as well to ensure that it is
aligning to what we want. Then we should add aligned (4) everywhere as mostly we
need 4 byte aligned here

-- 
~Vinod

Re: [PATCH] ALSA: compress: Fix the mismatch size of struc between share lib(32bit) and kernel(64bit)

[Arnd Bergmann]

On Tuesday 17 June 2014, Vinod Koul wrote:
> > Anyway, if you use the __packed attribute, best apply it only to
> > the individual __u64 member(s), not the entire struct, otherwise
> > you might change user space programs in a subtle way when the alignment
> > changes from 4 to 1 byte. 
>
> then wouldn't it make sense to call out the aligned as well to ensure that it is
> aligning to what we want. Then we should add aligned (4) everywhere as mostly we
> need 4 byte aligned here

If you want to be explicit, then just mark the entire structure as
attribute((packed,aligned(4))), not each individual member.

	Arnd

Re: [PATCH] ALSA: compress: Fix the mismatch size of struc between share lib(32bit) and kernel(64bit)

[Vinod Koul]

On Tue, Jun 17, 2014 at 01:48:59PM +0200, Arnd Bergmann wrote:
> On Tuesday 17 June 2014, Vinod Koul wrote:
> > > Anyway, if you use the __packed attribute, best apply it only to
> > > the individual __u64 member(s), not the entire struct, otherwise
> > > you might change user space programs in a subtle way when the alignment
> > > changes from 4 to 1 byte. 
> >
> > then wouldn't it make sense to call out the aligned as well to ensure that it is
> > aligning to what we want. Then we should add aligned (4) everywhere as mostly we
> > need 4 byte aligned here
> 
> If you want to be explicit, then just mark the entire structure as
> attribute((packed,aligned(4))), not each individual member.
Oh yes, thats the idea. I will send out and also while reviewing found that
packed was missing for few of other structs, so lets fix it now before 64bits
systems proliferate and cause havoc :)

-- 
~Vinod