| Message ID | ZSRvh1j2MVVhuOUv@work (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 4f88c72b2479cca4a0d4de89b4cbb6f1b37ee96d |
| Headers | show |
| Series | [next] ASoC: sigmadsp: Add __counted_by for struct sigmadsp_data and use struct_size() | expand |
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
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); if (!data) return -ENOMEM;
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(-)