[next] ASoC: sigmadsp: Add __counted_by for struct sigmadsp_data and use struct_size()

  Prepare for the coming implementation by GCC and Clang of the __counted_by
attribute. Flexible array members annotated with __counted_by can have
their accesses bounds-checked at run-time via CONFIG_UBSAN_BOUNDS (for
array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family
functions).

While there, use struct_size() and size_sub() helpers, instead of the
open-coded version, to calculate the size for the allocation of the
whole flexible structure, including of course, the flexible-array
member.

This code was found with the help of Coccinelle, and audited and
fixed manually.

Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
---
 sound/soc/codecs/sigmadsp.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)
  

On Mon, Oct 09, 2023 at 03:24:23PM -0600, Gustavo A. R. Silva wrote:
> Prepare for the coming implementation by GCC and Clang of the __counted_by
> attribute. Flexible array members annotated with __counted_by can have
> their accesses bounds-checked at run-time via CONFIG_UBSAN_BOUNDS (for
> array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family
> functions).
> 
> While there, use struct_size() and size_sub() helpers, instead of the
> open-coded version, to calculate the size for the allocation of the
> whole flexible structure, including of course, the flexible-array
> member.
> 
> This code was found with the help of Coccinelle, and audited and
> fixed manually.
> 
> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
> ---
>  sound/soc/codecs/sigmadsp.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/sound/soc/codecs/sigmadsp.c b/sound/soc/codecs/sigmadsp.c
> index b93c078a8040..56546e2394ab 100644
> --- a/sound/soc/codecs/sigmadsp.c
> +++ b/sound/soc/codecs/sigmadsp.c
> @@ -43,7 +43,7 @@ struct sigmadsp_data {
>  	uint32_t samplerates;
>  	unsigned int addr;
>  	unsigned int length;
> -	uint8_t data[];
> +	uint8_t data[] __counted_by(length);
>  };
>  
>  struct sigma_fw_chunk {
> @@ -270,7 +270,7 @@ static int sigma_fw_load_data(struct sigmadsp *sigmadsp,
>  
>  	length -= sizeof(*data_chunk);
>  
> -	data = kzalloc(sizeof(*data) + length, GFP_KERNEL);
> +	data = kzalloc(struct_size(data, data, length), GFP_KERNEL);
>  	if (!data)
>  		return -ENOMEM;
>  
> @@ -413,7 +413,8 @@ static int process_sigma_action(struct sigmadsp *sigmadsp,
>  		if (len < 3)
>  			return -EINVAL;
>  
> -		data = kzalloc(sizeof(*data) + len - 2, GFP_KERNEL);
> +		data = kzalloc(struct_size(data, data, size_sub(len, 2)),
> +			       GFP_KERNEL);

Since len was just size-checked before the alloc, size_sub() is a bit of
overkill, but it's not technically wrong. :P

Reviewed-by: Kees Cook <keescook@chromium.org>
  

On 10/10/23 00:03, Kees Cook wrote:
> On Mon, Oct 09, 2023 at 03:24:23PM -0600, Gustavo A. R. Silva wrote:
>> Prepare for the coming implementation by GCC and Clang of the __counted_by
>> attribute. Flexible array members annotated with __counted_by can have
>> their accesses bounds-checked at run-time via CONFIG_UBSAN_BOUNDS (for
>> array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family
>> functions).
>>
>> While there, use struct_size() and size_sub() helpers, instead of the
>> open-coded version, to calculate the size for the allocation of the
>> whole flexible structure, including of course, the flexible-array
>> member.
>>
>> This code was found with the help of Coccinelle, and audited and
>> fixed manually.
>>
>> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
>> ---
>>   sound/soc/codecs/sigmadsp.c | 7 ++++---
>>   1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/sound/soc/codecs/sigmadsp.c b/sound/soc/codecs/sigmadsp.c
>> index b93c078a8040..56546e2394ab 100644
>> --- a/sound/soc/codecs/sigmadsp.c
>> +++ b/sound/soc/codecs/sigmadsp.c
>> @@ -43,7 +43,7 @@ struct sigmadsp_data {
>>   	uint32_t samplerates;
>>   	unsigned int addr;
>>   	unsigned int length;
>> -	uint8_t data[];
>> +	uint8_t data[] __counted_by(length);
>>   };
>>   
>>   struct sigma_fw_chunk {
>> @@ -270,7 +270,7 @@ static int sigma_fw_load_data(struct sigmadsp *sigmadsp,
>>   
>>   	length -= sizeof(*data_chunk);
>>   
>> -	data = kzalloc(sizeof(*data) + length, GFP_KERNEL);
>> +	data = kzalloc(struct_size(data, data, length), GFP_KERNEL);
>>   	if (!data)
>>   		return -ENOMEM;
>>   
>> @@ -413,7 +413,8 @@ static int process_sigma_action(struct sigmadsp *sigmadsp,
>>   		if (len < 3)
>>   			return -EINVAL;
>>   
>> -		data = kzalloc(sizeof(*data) + len - 2, GFP_KERNEL);
>> +		data = kzalloc(struct_size(data, data, size_sub(len, 2)),
>> +			       GFP_KERNEL);
> 
> Since len was just size-checked before the alloc, size_sub() is a bit of
> overkill, but it's not technically wrong. :P

Oops.. yep, you're right, I totally overlooked that check.

> 
> Reviewed-by: Kees Cook <keescook@chromium.org>
> 

Thanks!
--
Gustavo
  

On Mon, 09 Oct 2023 15:24:23 -0600, Gustavo A. R. Silva wrote:
> Prepare for the coming implementation by GCC and Clang of the __counted_by
> attribute. Flexible array members annotated with __counted_by can have
> their accesses bounds-checked at run-time via CONFIG_UBSAN_BOUNDS (for
> array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family
> functions).
> 
> While there, use struct_size() and size_sub() helpers, instead of the
> open-coded version, to calculate the size for the allocation of the
> whole flexible structure, including of course, the flexible-array
> member.
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next

Thanks!

[1/1] ASoC: sigmadsp: Add __counted_by for struct sigmadsp_data and use struct_size()
      commit: 4f88c72b2479cca4a0d4de89b4cbb6f1b37ee96d

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark