Re: [Sound-open-firmware] [alsa-devel] [PATCH v3 09/14] ASoC: SOF: Add firmware, loader support
![](https://secure.gravatar.com/avatar/1f3d34808c5f863f3f587f9b6a4c0291.jpg?s=120&d=mm&r=g)
Date: Wed, 12 Dec 2018 12:23:33 +0100 From: Takashi Iwai tiwai@suse.de To: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Cc: alsa-devel@alsa-project.org, andriy.shevchenko@intel.com, Daniel Baluta daniel.baluta@gmail.com, liam.r.girdwood@linux.intel.com, vkoul@kernel.org, broonie@kernel.org, Alan Cox alan@linux.intel.com, sound-open-firmware@alsa-project.org Subject: Re: [alsa-devel] [PATCH v3 09/14] ASoC: SOF: Add firmware loader support Message-ID: s5htvjjj8ei.wl-tiwai@suse.de Content-Type: text/plain; charset=US-ASCII
On Tue, 11 Dec 2018 22:23:13 +0100, Pierre-Louis Bossart wrote:
snd_sof_dsp_block_write(sdev, offset,
(void *)block + sizeof(*block),
block->size);
/* next block */
block = (void *)block + sizeof(*block) + block->size;
This may lead to an unaligned access.
Did you mean we should double check the block->size to prevent access to an invalid address?
Also how is the endianess guaranteed?
Did you mean we should guarantee the driver can work no matter what kernel's endianess is? ie. Use le32_to_cpu() to handle it?
thanks,
Takashi
![](https://secure.gravatar.com/avatar/5b19e9d0e834ea10ef75803718ad564b.jpg?s=120&d=mm&r=g)
On Thu, 20 Dec 2018 03:11:55 +0100, Bard liao wrote:
Date: Wed, 12 Dec 2018 12:23:33 +0100 From: Takashi Iwai tiwai@suse.de To: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Cc: alsa-devel@alsa-project.org, andriy.shevchenko@intel.com, Daniel Baluta daniel.baluta@gmail.com, liam.r.girdwood@linux.intel.com, vkoul@kernel.org, broonie@kernel.org, Alan Cox alan@linux.intel.com, sound-open-firmware@alsa-project.org Subject: Re: [alsa-devel] [PATCH v3 09/14] ASoC: SOF: Add firmware loader support Message-ID: s5htvjjj8ei.wl-tiwai@suse.de Content-Type: text/plain; charset=US-ASCII
On Tue, 11 Dec 2018 22:23:13 +0100, Pierre-Louis Bossart wrote:
snd_sof_dsp_block_write(sdev, offset,
(void *)block + sizeof(*block),
block->size);
/* next block */
block = (void *)block + sizeof(*block) + block->size;
This may lead to an unaligned access.
Did you mean we should double check the block->size to prevent access to an invalid address?
You need two types of checks for the given data: - The bounce check of block->size; We need to avoid out-of-bounce access.
- Alignment of block->size; For some non-x86 platforms, the access to an unaligned address might be illegal.
Oh, and recently another thing is sometimes needed for avoiding Spectre. This can be covered by array_index_nospec().
Also how is the endianess guaranteed?
Did you mean we should guarantee the driver can work no matter what kernel's endianess is? ie. Use le32_to_cpu() to handle it?
Depends on the implementation. IIRC, topology API refuses the data in a different endianess by checking the magic number at beginning.
thanks,
Takashi
![](https://secure.gravatar.com/avatar/59bffa8d29505a5951521a2fd09a4fd9.jpg?s=120&d=mm&r=g)
snd_sof_dsp_block_write(sdev, offset,
(void *)block + sizeof(*block),
block->size);
/* next block */
block = (void *)block + sizeof(*block) + block->size;
This may lead to an unaligned access.
Did you mean we should double check the block->size to prevent access to an invalid address?
You need two types of checks for the given data:
- The bounce check of block->size; We need to avoid out-of-bounce access.
s/bounce/bounds ?
- Alignment of block->size; For some non-x86 platforms, the access to an unaligned address might be illegal.
Maybe I am missing something but I don't see any sort of explicit restriction on alignment in the SOF tools. it looks implicit based on address offsets and bases.
Liam, do you see any negative side effects if we enforce a 32-bit alignment for all blocks (which essentially means all block sizes are multiple of 4)? we can try and experiment but it's better if we have an agreement on the design.
Oh, and recently another thing is sometimes needed for avoiding Spectre. This can be covered by array_index_nospec().
Also how is the endianess guaranteed?
Did you mean we should guarantee the driver can work no matter what kernel's endianess is? ie. Use le32_to_cpu() to handle it?
Depends on the implementation. IIRC, topology API refuses the data in a different endianess by checking the magic number at beginning.
snd_sof_dsp_block_write() is implemented with a platform-specific callback, I'd expect any endianess issues to be handled in that platform-specific code?
![](https://secure.gravatar.com/avatar/1f3d34808c5f863f3f587f9b6a4c0291.jpg?s=120&d=mm&r=g)
On 12/20/2018 11:07 PM, Pierre-Louis Bossart wrote:
- snd_sof_dsp_block_write(sdev, offset,
+ (void *)block + sizeof(*block), + block->size);
+ /* next block */ + block = (void *)block + sizeof(*block) + block->size;
This may lead to an unaligned access.
Did you mean we should double check the block->size to prevent access to an invalid address?
You need two types of checks for the given data:
- The bounce check of block->size;
We need to avoid out-of-bounce access.
s/bounce/bounds ?
- Alignment of block->size;
For some non-x86 platforms, the access to an unaligned address might be illegal.
Maybe I am missing something but I don't see any sort of explicit restriction on alignment in the SOF tools. it looks implicit based on address offsets and bases.
Maybe get_unaligned() is good enough to avoid unaligned access?
Liam, do you see any negative side effects if we enforce a 32-bit alignment for all blocks (which essentially means all block sizes are multiple of 4)? we can try and experiment but it's better if we have an agreement on the design.
![](https://secure.gravatar.com/avatar/5b19e9d0e834ea10ef75803718ad564b.jpg?s=120&d=mm&r=g)
On Fri, 21 Dec 2018 10:05:04 +0100, Bard liao wrote:
On 12/20/2018 11:07 PM, Pierre-Louis Bossart wrote:
- snd_sof_dsp_block_write(sdev, offset,
+ (void *)block + sizeof(*block), + block->size);
+ /* next block */ + block = (void *)block + sizeof(*block) + block->size;
This may lead to an unaligned access.
Did you mean we should double check the block->size to prevent access to an invalid address?
You need two types of checks for the given data:
- The bounce check of block->size;
We need to avoid out-of-bounce access.
s/bounce/bounds ?
- Alignment of block->size;
For some non-x86 platforms, the access to an unaligned address might be illegal.
Maybe I am missing something but I don't see any sort of explicit restriction on alignment in the SOF tools. it looks implicit based on address offsets and bases.
Maybe get_unaligned() is good enough to avoid unaligned access?
That's another option, but you'd need to put everywhere.
An alternative is to just copy the data on a new block header on the stack temporarily and refer it.
Or simply assuring the alignment by checking block->size as Pierre suggested...
Takashi
Liam, do you see any negative side effects if we enforce a 32-bit alignment for all blocks (which essentially means all block sizes are multiple of 4)? we can try and experiment but it's better if we have an agreement on the design.
![](https://secure.gravatar.com/avatar/24e6a8158be3c9b52253d878d1487123.jpg?s=120&d=mm&r=g)
On Fri, 2018-12-21 at 10:57 +0100, Takashi Iwai wrote:
On Fri, 21 Dec 2018 10:05:04 +0100, Bard liao wrote:
On 12/20/2018 11:07 PM, Pierre-Louis Bossart wrote:
> + snd_sof_dsp_block_write(sdev, offset, > + (void *)block + sizeof(*block), > + block->size); > + > + /* next block */ > + block = (void *)block + sizeof(*block) + block->size; This may lead to an unaligned access.
Did you mean we should double check the block->size to prevent access to an invalid address?
You need two types of checks for the given data:
- The bounce check of block->size; We need to avoid out-of-bounce access.
s/bounce/bounds ?
- Alignment of block->size; For some non-x86 platforms, the access to an unaligned address might be illegal.
Maybe I am missing something but I don't see any sort of explicit restriction on alignment in the SOF tools. it looks implicit based on address offsets and bases.
Maybe get_unaligned() is good enough to avoid unaligned access?
That's another option, but you'd need to put everywhere.
An alternative is to just copy the data on a new block header on the stack temporarily and refer it.
Or simply assuring the alignment by checking block->size as Pierre suggested...
Yep, checking size is good for me too.
Takashi
Liam, do you see any negative side effects if we enforce a 32-bit alignment for all blocks (which essentially means all block sizes are multiple of 4)? we can try and experiment but it's better if we have an agreement on the design.
This shouldn't cause any problems, iirc all block sizes are rounded up to nearest uint32_t anyway (as we have exception handler blocks that are 3 bytes of assembly in size and rounded up to 4).
Liam
Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
participants (4)
-
Bard liao
-
Liam Girdwood
-
Pierre-Louis Bossart
-
Takashi Iwai