Andy, Kees,
On Wed, Apr 16, 2025 at 09:45:52AM +0300, Andy Shevchenko wrote:
On Tue, Apr 15, 2025 at 04:14:24PM -0700, Kees Cook wrote:
The 20 byte length of struct platform_device_id::name is not long enough for many devices (especially regulators), where the string initialization is getting truncated and missing the trailing NUL byte. This is seen with GCC 15's -Wunterminated-string-initialization option:
drivers/regulator/hi6421v530-regulator.c:189:19: warning: initializer-string for array of 'char' truncates NUL terminator but destination lacks 'nonstring' attribute (21 chars into 20 available) [-Wunterminated-string-initialization] 189 | { .name = "hi6421v530-regulator" }, | ^~~~~~~~~~~~~~~~~~~~~~ drivers/regulator/hi6421v600-regulator.c:278:19: warning: initializer-string for array of 'char' truncates NUL terminator but destination lacks 'nonstring' attribute (21 chars into 20 available) [-Wunterminated-string-initialization] 278 | { .name = "hi6421v600-regulator" }, | ^~~~~~~~~~~~~~~~~~~~~~ drivers/regulator/lp87565-regulator.c:233:11: warning: initializer-string for array of 'char' truncates NUL terminator but destination lacks 'nonstring' attribute (21 chars into 20 available) [-Wunterminated-string-initialization] 233 | { "lp87565-q1-regulator", }, | ^~~~~~~~~~~~~~~~~~~~~~ sound/soc/fsl/imx-pcm-rpmsg.c:818:19: warning: initializer-string for array of 'char' truncates NUL terminator but destination lacks 'nonstring' attribute (21 chars into 20 available) [-Wunterminated-string-initialization] 818 | { .name = "rpmsg-micfil-channel" }, | ^~~~~~~~~~~~~~~~~~~~~~ drivers/iio/light/hid-sensor-als.c:457:25: warning: initializer-string for array of 'char' truncates NUL terminator but destination lacks 'nonstring' attribute (21 chars into 20 available) [-Wunterminated-string-initialization] 457 | .name = "HID-SENSOR-LISS-0041", | ^~~~~~~~~~~~~~~~~~~~~~ drivers/iio/light/hid-sensor-prox.c:366:25: warning: initializer-string for array of 'char' truncates NUL terminator but destination lacks 'nonstring' attribute (21 chars into 20 available) [-Wunterminated-string-initialization] 366 | .name = "HID-SENSOR-LISS-0226", | ^~~~~~~~~~~~~~~~~~~~~~
Increase the length to 24, slightly more than is currently being used by the affected drivers. The string is used in '%s' format strings and via the module code, which appears to do its own length encoding. This size was chosen because there was already a 4 byte hole in the structure:
struct platform_device_id { char name[20]; /* 0 20 */
/* XXX 4 bytes hole, try to pack */ kernel_ulong_t driver_data; /* 24 8 */ /* size: 32, cachelines: 1, members: 2 */ /* sum members: 28, holes: 1, sum holes: 4 */ /* last cacheline: 32 bytes */};
Since there is no even potential ABI breakage, I'm fine with the change. Reviewed-by: Andy Shevchenko andriy.shevchenko@linux.intel.com
This definitely breaks ABI on 32-bit architectures such as i586, because there is no gap from alignment. Perhaps, this also make the commit not suitable for backporting to stable branches?
I recently stumbled on build failure on v5.10.239 for i586:
make: Entering directory '/usr/src/kernel-source-5.10' DEPMOD 5.10.239 depmod: FATAL: Module index: bad character '�'=0x80 - only 7-bit ASCII is supported: platform:jsl_rt5682_max98360ax� make: *** [Makefile:1786: modules_install] Error 1 make: Leaving directory '/usr/src/kernel-source-5.10'
With this patch not applied "jsl_rt5682_max98360a" have terminating '\0' truncated due to PLATFORM_NAME_SIZE being same as the string length and concatenated with the following binary data:
{ .name = "jsl_rt5682_max98360a", .driver_data = (kernel_ulong_t)(SOF_RT5682_MCLK_EN | SOF_RT5682_MCLK_24MHZ | SOF_RT5682_SSP_CODEC(0) | SOF_SPEAKER_AMP_PRESENT | SOF_MAX98360A_SPEAKER_AMP_PRESENT | SOF_RT5682_SSP_AMP(1)), },
modpost then interprets it as an asciiz string concatenating with `driver_data` resulting in bad characters.
static int do_platform_entry(const char *filename, void *symval, char *alias) { DEF_FIELD_ADDR(symval, platform_device_id, name); sprintf(alias, PLATFORM_MODULE_PREFIX "%s", *name); return 1; }
creating in an incorrect alias, and this somehow breaks depmod in kmod 34.2 (maybe earlier).
Old kmod 30 successfully adds incorrect alias:
$ modinfo snd-soc-sof_rt5682.ko | grep jsl_rt5682_max98360a alias: platform:jsl_rt5682_max98360a alias: platform:jsl_rt5682_max98360ax�
and
modules.alias:alias platform:jsl_rt5682_max98360ax� snd_soc_sof_rt5682
Perhaps, scripts/mod/file2alias.c should be updated with:
- sprintf(alias, PLATFORM_MODULE_PREFIX "%s", *name); + sprintf(alias, PLATFORM_MODULE_PREFIX "%.*s", PLATFORM_NAME_SIZE, *name);
(Or even producing an error if more serious truncation occurs.)
Thanks,
-- With Best Regards, Andy Shevchenko