[PATCH 00/12] Clang -Wformat warning fixes
This patch set fixes some clang warnings when -Wformat is enabled.
Bill Wendling (12): x86/mce: use correct format characters x86/CPU/AMD: use correct format characters x86/e820: use correct format characters blk-cgroup: use correct format characters fs: quota: use correct format characters PNP: use correct format characters driver/char: use correct format characters cdrom: use correct format characters ALSA: seq: use correct format characters ALSA: seq: use correct format characters ALSA: control: use correct format characters netfilter: conntrack: use correct format characters
arch/x86/kernel/cpu/mce/amd.c | 9 +++++---- arch/x86/kernel/cpu/mce/core.c | 2 +- arch/x86/kernel/e820.c | 4 ++-- drivers/cdrom/cdrom.c | 2 +- drivers/char/mem.c | 2 +- drivers/pnp/interface.c | 2 +- fs/quota/dquot.c | 2 +- mm/backing-dev.c | 2 +- net/netfilter/nf_conntrack_helper.c | 2 +- scripts/Makefile.extrawarn | 4 ++-- sound/core/control.c | 2 +- sound/core/seq/seq_clientmgr.c | 2 +- sound/core/sound.c | 2 +- 13 files changed, 19 insertions(+), 18 deletions(-)
From: Bill Wendling isanbard@gmail.com
When compiling with -Wformat, clang emits the following warnings:
arch/x86/kernel/cpu/mce/core.c:295:9: error: format string is not a string literal (potentially insecure) [-Werror,-Wformat-security] panic(msg); ^~~
Use a string literal for the format string.
Link: https://github.com/ClangBuiltLinux/linux/issues/378 Signed-off-by: Bill Wendling isanbard@gmail.com --- arch/x86/kernel/cpu/mce/core.c | 2 +- scripts/Makefile.extrawarn | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c index 2c8ec5c71712..3d411b7c85ad 100644 --- a/arch/x86/kernel/cpu/mce/core.c +++ b/arch/x86/kernel/cpu/mce/core.c @@ -292,7 +292,7 @@ static noinstr void mce_panic(const char *msg, struct mce *final, char *exp) if (!fake_panic) { if (panic_timeout == 0) panic_timeout = mca_cfg.panic_timeout; - panic(msg); + panic("%s", msg); } else pr_emerg(HW_ERR "Fake kernel panic: %s\n", msg);
On 6/9/22 15:16, Bill Wendling wrote:
From: Bill Wendling isanbard@gmail.com
When compiling with -Wformat, clang emits the following warnings:
arch/x86/kernel/cpu/mce/core.c:295:9: error: format string is not a string literal (potentially insecure) [-Werror,-Wformat-security] panic(msg); ^~~
Use a string literal for the format string.
Link: https://github.com/ClangBuiltLinux/linux/issues/378 Signed-off-by: Bill Wendling isanbard@gmail.com
arch/x86/kernel/cpu/mce/core.c | 2 +- scripts/Makefile.extrawarn | 4 ++--
Where is the scripts/ change?
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c index 2c8ec5c71712..3d411b7c85ad 100644 --- a/arch/x86/kernel/cpu/mce/core.c +++ b/arch/x86/kernel/cpu/mce/core.c @@ -292,7 +292,7 @@ static noinstr void mce_panic(const char *msg, struct mce *final, char *exp) if (!fake_panic) { if (panic_timeout == 0) panic_timeout = mca_cfg.panic_timeout;
panic(msg);
panic("%s", msg);
On Thu, Jun 9, 2022 at 4:15 PM Randy Dunlap rdunlap@infradead.org wrote:
On 6/9/22 15:16, Bill Wendling wrote:
From: Bill Wendling isanbard@gmail.com
When compiling with -Wformat, clang emits the following warnings:
arch/x86/kernel/cpu/mce/core.c:295:9: error: format string is not a string literal (potentially insecure) [-Werror,-Wformat-security] panic(msg); ^~~
Use a string literal for the format string.
Link: https://github.com/ClangBuiltLinux/linux/issues/378 Signed-off-by: Bill Wendling isanbard@gmail.com
arch/x86/kernel/cpu/mce/core.c | 2 +- scripts/Makefile.extrawarn | 4 ++--
Where is the scripts/ change?
I'm sorry about this. The change in that file was a mistake that I reverted, but I forgot to change this part.
-bw
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c index 2c8ec5c71712..3d411b7c85ad 100644 --- a/arch/x86/kernel/cpu/mce/core.c +++ b/arch/x86/kernel/cpu/mce/core.c @@ -292,7 +292,7 @@ static noinstr void mce_panic(const char *msg, struct mce *final, char *exp) if (!fake_panic) { if (panic_timeout == 0) panic_timeout = mca_cfg.panic_timeout;
panic(msg);
panic("%s", msg); } else pr_emerg(HW_ERR "Fake kernel panic: %s\n", msg);-- ~Randy
From: Bill Wendling isanbard@gmail.com
When compiling with -Wformat, clang emits the following warnings:
arch/x86/kernel/cpu/mce/amd.c:1119:67: error: format string is not a string literal (potentially insecure) [-Werror,-Wformat-security] err = kobject_init_and_add(&b->kobj, &threshold_ktype, tb->kobj, get_name(cpu, bank, b)); ^~~~~~~~~~~~~~~~~~~~~~ arch/x86/kernel/cpu/mce/amd.c:1151:47: error: format string is not a string literal (potentially insecure) [-Werror,-Wformat-security] err = kobject_add(&b->blocks->kobj, b->kobj, b->blocks->kobj.name); ^~~~~~~~~~~~~~~~~~~~ arch/x86/kernel/cpu/mce/amd.c:1157:42: error: format string is not a string literal (potentially insecure) [-Werror,-Wformat-security] err = kobject_add(&pos->kobj, b->kobj, pos->kobj.name); ^~~~~~~~~~~~~~ arch/x86/kernel/cpu/mce/amd.c:1187:43: error: format string is not a string literal (potentially insecure) [-Werror,-Wformat-security] err = kobject_add(b->kobj, &dev->kobj, name); ^~~~ "%s",
Use a string literal for the format string.
Link: https://github.com/ClangBuiltLinux/linux/issues/378 Signed-off-by: Bill Wendling isanbard@gmail.com --- arch/x86/kernel/cpu/mce/amd.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/arch/x86/kernel/cpu/mce/amd.c b/arch/x86/kernel/cpu/mce/amd.c index 1c87501e0fa3..d19bf0eb0abe 100644 --- a/arch/x86/kernel/cpu/mce/amd.c +++ b/arch/x86/kernel/cpu/mce/amd.c @@ -1116,7 +1116,8 @@ static int allocate_threshold_blocks(unsigned int cpu, struct threshold_bank *tb else tb->blocks = b;
- err = kobject_init_and_add(&b->kobj, &threshold_ktype, tb->kobj, get_name(cpu, bank, b)); + err = kobject_init_and_add(&b->kobj, &threshold_ktype, tb->kobj, "%s", + get_name(cpu, bank, b)); if (err) goto out_free; recurse: @@ -1148,13 +1149,13 @@ static int __threshold_add_blocks(struct threshold_bank *b) struct threshold_block *tmp = NULL; int err = 0;
- err = kobject_add(&b->blocks->kobj, b->kobj, b->blocks->kobj.name); + err = kobject_add(&b->blocks->kobj, b->kobj, "%s", b->blocks->kobj.name); if (err) return err;
list_for_each_entry_safe(pos, tmp, head, miscj) {
- err = kobject_add(&pos->kobj, b->kobj, pos->kobj.name); + err = kobject_add(&pos->kobj, b->kobj, "%s", pos->kobj.name); if (err) { list_for_each_entry_safe_reverse(pos, tmp, head, miscj) kobject_del(&pos->kobj); @@ -1184,7 +1185,7 @@ static int threshold_create_bank(struct threshold_bank **bp, unsigned int cpu, if (nb && nb->bank4) { /* yes, use it */ b = nb->bank4; - err = kobject_add(b->kobj, &dev->kobj, name); + err = kobject_add(b->kobj, &dev->kobj, "%s", name); if (err) goto out;
From: Bill Wendling isanbard@gmail.com
When compiling with -Wformat, clang emits the following warnings:
arch/x86/kernel/e820.c:877:15: error: format string is not a string literal (potentially insecure) [-Werror,-Wformat-security] early_printk(msg); ^~~ arch/x86/kernel/e820.c:878:8: error: format string is not a string literal (potentially insecure) [-Werror,-Wformat-security] panic(msg); ^~~
Use a string literal for the format string.
Link: https://github.com/ClangBuiltLinux/linux/issues/378 Signed-off-by: Bill Wendling isanbard@gmail.com --- arch/x86/kernel/e820.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/x86/kernel/e820.c b/arch/x86/kernel/e820.c index f267205f2d5a..ca4634a0bdb5 100644 --- a/arch/x86/kernel/e820.c +++ b/arch/x86/kernel/e820.c @@ -874,8 +874,8 @@ unsigned long __init e820__end_of_low_ram_pfn(void)
static void __init early_panic(char *msg) { - early_printk(msg); - panic(msg); + early_printk("%s", msg); + panic("%s", msg); }
static int userdef __initdata;
From: Bill Wendling isanbard@gmail.com
When compiling with -Wformat, clang emits the following warnings:
mm/backing-dev.c:880:57: error: format string is not a string literal (potentially insecure) [-Werror,-Wformat-security] dev = device_create(bdi_class, NULL, MKDEV(0, 0), bdi, bdi->dev_name); ^~~~~~~~~~~~~
Use a string literal for the format string.
Link: https://github.com/ClangBuiltLinux/linux/issues/378 Signed-off-by: Bill Wendling isanbard@gmail.com --- mm/backing-dev.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/mm/backing-dev.c b/mm/backing-dev.c index ff60bd7d74e0..7b7786dceff3 100644 --- a/mm/backing-dev.c +++ b/mm/backing-dev.c @@ -877,7 +877,7 @@ int bdi_register_va(struct backing_dev_info *bdi, const char *fmt, va_list args) return 0;
vsnprintf(bdi->dev_name, sizeof(bdi->dev_name), fmt, args); - dev = device_create(bdi_class, NULL, MKDEV(0, 0), bdi, bdi->dev_name); + dev = device_create(bdi_class, NULL, MKDEV(0, 0), bdi, "%s", bdi->dev_name); if (IS_ERR(dev)) return PTR_ERR(dev);
On Thu, Jun 09, 2022 at 10:16:23PM +0000, Bill Wendling wrote:
vsnprintf(bdi->dev_name, sizeof(bdi->dev_name), fmt, args);
- dev = device_create(bdi_class, NULL, MKDEV(0, 0), bdi, bdi->dev_name);
- dev = device_create(bdi_class, NULL, MKDEV(0, 0), bdi, "%s", bdi->dev_name);
Please avoid the overly long line. But given that bdi names aren't user input this warning seems to shoot from the hip a bit.
From: Bill Wendling isanbard@gmail.com
When compiling with -Wformat, clang emits the following warnings:
fs/quota/dquot.c:206:22: error: format string is not a string literal (potentially insecure) [-Werror,-Wformat-security] request_module(module_names[qm].qm_mod_name)) ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
Use a string literal for the format string.
Link: https://github.com/ClangBuiltLinux/linux/issues/378 Signed-off-by: Bill Wendling isanbard@gmail.com --- fs/quota/dquot.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/quota/dquot.c b/fs/quota/dquot.c index a74aef99bd3d..3b613de3b371 100644 --- a/fs/quota/dquot.c +++ b/fs/quota/dquot.c @@ -203,7 +203,7 @@ static struct quota_format_type *find_quota_format(int id) module_names[qm].qm_fmt_id != id; qm++) ; if (!module_names[qm].qm_fmt_id || - request_module(module_names[qm].qm_mod_name)) + request_module("%s", module_names[qm].qm_mod_name)) return NULL;
spin_lock(&dq_list_lock);
From: Bill Wendling isanbard@gmail.com
When compiling with -Wformat, clang emits the following warnings:
drivers/pnp/interface.c:273:22: error: format string is not a string literal (potentially insecure) [-Werror,-Wformat-security] pnp_printf(buffer, pnp_resource_type_name(res)); ^~~~~~~~~~~~~~~~~~~~~~~~~~~
Use a string literal for the format string.
Link: https://github.com/ClangBuiltLinux/linux/issues/378 Signed-off-by: Bill Wendling isanbard@gmail.com --- drivers/pnp/interface.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/pnp/interface.c b/drivers/pnp/interface.c index 44efcdb87e6f..553221a0c89a 100644 --- a/drivers/pnp/interface.c +++ b/drivers/pnp/interface.c @@ -270,7 +270,7 @@ static ssize_t resources_show(struct device *dmdev, list_for_each_entry(pnp_res, &dev->resources, list) { res = &pnp_res->res;
- pnp_printf(buffer, pnp_resource_type_name(res)); + pnp_printf(buffer, "%s", pnp_resource_type_name(res));
if (res->flags & IORESOURCE_DISABLED) { pnp_printf(buffer, " disabled\n");
From: Bill Wendling isanbard@gmail.com
When compiling with -Wformat, clang emits the following warnings:
drivers/char/mem.c:775:16: error: format string is not a string literal (potentially insecure) [-Werror,-Wformat-security] NULL, devlist[minor].name); ^~~~~~~~~~~~~~~~~~~
Use a string literal for the format string.
Link: https://github.com/ClangBuiltLinux/linux/issues/378 Signed-off-by: Bill Wendling isanbard@gmail.com --- drivers/char/mem.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/char/mem.c b/drivers/char/mem.c index 84ca98ed1dad..32d821ba9e4d 100644 --- a/drivers/char/mem.c +++ b/drivers/char/mem.c @@ -772,7 +772,7 @@ static int __init chr_dev_init(void) continue;
device_create(mem_class, NULL, MKDEV(MEM_MAJOR, minor), - NULL, devlist[minor].name); + NULL, "%s", devlist[minor].name); }
return tty_init();
On Thu, Jun 09, 2022 at 10:16:26PM +0000, Bill Wendling wrote:
From: Bill Wendling isanbard@gmail.com
Why isn't that matching your From: line in the email?
When compiling with -Wformat, clang emits the following warnings:
Is that ever a default build option for the kernel?
drivers/char/mem.c:775:16: error: format string is not a string literal (potentially insecure) [-Werror,-Wformat-security] NULL, devlist[minor].name); ^~~~~~~~~~~~~~~~~~~
Use a string literal for the format string.
Link: https://github.com/ClangBuiltLinux/linux/issues/378 Signed-off-by: Bill Wendling isanbard@gmail.com
drivers/char/mem.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/char/mem.c b/drivers/char/mem.c index 84ca98ed1dad..32d821ba9e4d 100644 --- a/drivers/char/mem.c +++ b/drivers/char/mem.c @@ -772,7 +772,7 @@ static int __init chr_dev_init(void) continue;
device_create(mem_class, NULL, MKDEV(MEM_MAJOR, minor),
NULL, devlist[minor].name);
NULL, "%s", devlist[minor].name);
Please explain how this static string can ever be user controlled.
thanks,
greg k-h
On Thu, Jun 9, 2022 at 10:18 PM Greg Kroah-Hartman gregkh@linuxfoundation.org wrote:
On Thu, Jun 09, 2022 at 10:16:26PM +0000, Bill Wendling wrote:
From: Bill Wendling isanbard@gmail.com
Why isn't that matching your From: line in the email?
There must be something wrong with my .gitconfig file. I"ll check into it.
When compiling with -Wformat, clang emits the following warnings:
Is that ever a default build option for the kernel?
We want to enable -Wformat for clang. I believe that these specific warnings have been disabled, but I'm confused as to why, because they're valid warnings. When I compiled with the warning enabled, there were only a few (12) places that needed changes, so thought that patches would be a nice cleanup, even though the warning itself is disabled.
drivers/char/mem.c:775:16: error: format string is not a string literal (potentially insecure) [-Werror,-Wformat-security] NULL, devlist[minor].name); ^~~~~~~~~~~~~~~~~~~
Use a string literal for the format string.
Link: https://github.com/ClangBuiltLinux/linux/issues/378 Signed-off-by: Bill Wendling isanbard@gmail.com
drivers/char/mem.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/char/mem.c b/drivers/char/mem.c index 84ca98ed1dad..32d821ba9e4d 100644 --- a/drivers/char/mem.c +++ b/drivers/char/mem.c @@ -772,7 +772,7 @@ static int __init chr_dev_init(void) continue;
device_create(mem_class, NULL, MKDEV(MEM_MAJOR, minor),
NULL, devlist[minor].name);
NULL, "%s", devlist[minor].name);Please explain how this static string can ever be user controlled.
All someone would need to do is accidentally insert an errant '%' in one of the strings for this function call to perform unexpected actions---at the very least reading memory that's not allocated and may contain garbage, thereby decreasing performance and possibly overrunning some buffer. Perhaps in this specific scenario it's unlikely, but "device_create()" is used in a lot more places than here. This patch is a general code cleanup.
-bw
From: Bill Wendling isanbard@gmail.com
When compiling with -Wformat, clang emits the following warnings:
drivers/cdrom/cdrom.c:3454:48: error: format string is not a string literal (potentially insecure) [-Werror,-Wformat-security] ret = scnprintf(info + *pos, max_size - *pos, header); ^~~~~~
Use a string literal for the format string.
Link: https://github.com/ClangBuiltLinux/linux/issues/378 Signed-off-by: Bill Wendling isanbard@gmail.com --- drivers/cdrom/cdrom.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/cdrom/cdrom.c b/drivers/cdrom/cdrom.c index 416f723a2dbb..52b40120c76e 100644 --- a/drivers/cdrom/cdrom.c +++ b/drivers/cdrom/cdrom.c @@ -3451,7 +3451,7 @@ static int cdrom_print_info(const char *header, int val, char *info, struct cdrom_device_info *cdi; int ret;
- ret = scnprintf(info + *pos, max_size - *pos, header); + ret = scnprintf(info + *pos, max_size - *pos, "%s", header); if (!ret) return 1;
On Thu, Jun 09, 2022 at 10:16:27PM +0000, Bill Wendling wrote:
From: Bill Wendling isanbard@gmail.com
When compiling with -Wformat, clang emits the following warnings:
drivers/cdrom/cdrom.c:3454:48: error: format string is not a string literal (potentially insecure) [-Werror,-Wformat-security] ret = scnprintf(info + *pos, max_size - *pos, header); ^~~~~~
Use a string literal for the format string.
Link: https://github.com/ClangBuiltLinux/linux/issues/378 Signed-off-by: Bill Wendling isanbard@gmail.com
drivers/cdrom/cdrom.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/cdrom/cdrom.c b/drivers/cdrom/cdrom.c index 416f723a2dbb..52b40120c76e 100644 --- a/drivers/cdrom/cdrom.c +++ b/drivers/cdrom/cdrom.c @@ -3451,7 +3451,7 @@ static int cdrom_print_info(const char *header, int val, char *info, struct cdrom_device_info *cdi; int ret;
- ret = scnprintf(info + *pos, max_size - *pos, header);
- ret = scnprintf(info + *pos, max_size - *pos, "%s", header); if (!ret) return 1;
-- 2.36.1.255.ge46751e96f-goog
Hi Bill,
Thank you for the patch, much appreciated.
Looking at this though, all callers of cdrom_print_info() provide 'header' as a string literal defined within the driver, when making the call. Therefore, I'm not convinced this change is necessary for cdrom.c - that said, in this particular use case I don't think it would hurt either.
I've followed the other responses on parts of this series, so I understand that a different solution is potentially in the works. Thought I'd respond anyway though out of courtesy.
All the best, Phil (Uniform CDROM Maintainer)
On Sun, Jun 12, 2022 at 9:23 AM Phillip Potter phil@philpotter.co.uk wrote:
On Thu, Jun 09, 2022 at 10:16:27PM +0000, Bill Wendling wrote:
From: Bill Wendling isanbard@gmail.com
When compiling with -Wformat, clang emits the following warnings:
drivers/cdrom/cdrom.c:3454:48: error: format string is not a string literal (potentially insecure) [-Werror,-Wformat-security] ret = scnprintf(info + *pos, max_size - *pos, header); ^~~~~~
Use a string literal for the format string.
Link: https://github.com/ClangBuiltLinux/linux/issues/378 Signed-off-by: Bill Wendling isanbard@gmail.com
drivers/cdrom/cdrom.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/cdrom/cdrom.c b/drivers/cdrom/cdrom.c index 416f723a2dbb..52b40120c76e 100644 --- a/drivers/cdrom/cdrom.c +++ b/drivers/cdrom/cdrom.c @@ -3451,7 +3451,7 @@ static int cdrom_print_info(const char *header, int val, char *info, struct cdrom_device_info *cdi; int ret;
ret = scnprintf(info + *pos, max_size - *pos, header);
ret = scnprintf(info + *pos, max_size - *pos, "%s", header); if (!ret) return 1;-- 2.36.1.255.ge46751e96f-goog
Hi Bill,
Thank you for the patch, much appreciated.
Looking at this though, all callers of cdrom_print_info() provide 'header' as a string literal defined within the driver, when making the call. Therefore, I'm not convinced this change is necessary for cdrom.c - that said, in this particular use case I don't think it would hurt either.
I've followed the other responses on parts of this series, so I understand that a different solution is potentially in the works. Thought I'd respond anyway though out of courtesy.
Thanks, Phillip.
I pointed out in a separate response that this specific warning is disabled by default, but when I ran into while hacking stuff there weren't a lot of places where the warning popped up (at least for x86 builds) and thought it would be a nice cleanup. I understand if you don't think this patch is necessary for your code. There are some places where visual inspection of the code is "good enough" to ensure that nothing untoward will happen (Greg pointed out a similar thing in an mm/ file).
Cheers! -bw
From: Bill Wendling isanbard@gmail.com
When compiling with -Wformat, clang emits the following warnings:
sound/core/seq/seq_clientmgr.c:2414:22: error: format string is not a string literal (potentially insecure) [-Werror,-Wformat-security] snd_iprintf(buffer, msg); ^~~
Use a string literal for the format string.
Link: https://github.com/ClangBuiltLinux/linux/issues/378 Signed-off-by: Bill Wendling isanbard@gmail.com --- sound/core/seq/seq_clientmgr.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/core/seq/seq_clientmgr.c b/sound/core/seq/seq_clientmgr.c index 2e9d695d336c..2340f3e14eeb 100644 --- a/sound/core/seq/seq_clientmgr.c +++ b/sound/core/seq/seq_clientmgr.c @@ -2411,7 +2411,7 @@ static void snd_seq_info_dump_subscribers(struct snd_info_buffer *buffer, up_read(&group->list_mutex); return; } - snd_iprintf(buffer, msg); + snd_iprintf(buffer, "%s", msg); list_for_each(p, &group->list_head) { if (is_src) s = list_entry(p, struct snd_seq_subscribers, src_list);
From: Bill Wendling isanbard@gmail.com
When compiling with -Wformat, clang emits the following warnings:
sound/core/sound.c:79:17: error: format string is not a string literal (potentially insecure) [-Werror,-Wformat-security] request_module(str); ^~~
Use a string literal for the format string.
Link: https://github.com/ClangBuiltLinux/linux/issues/378 Signed-off-by: Bill Wendling isanbard@gmail.com --- sound/core/sound.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/core/sound.c b/sound/core/sound.c index df5571d98629..7866f29621bf 100644 --- a/sound/core/sound.c +++ b/sound/core/sound.c @@ -76,7 +76,7 @@ static void snd_request_other(int minor) case SNDRV_MINOR_TIMER: str = "snd-timer"; break; default: return; } - request_module(str); + request_module("%s", str); }
#endif /* modular kernel */
From: Bill Wendling isanbard@gmail.com
When compiling with -Wformat, clang emits the following warnings:
sound/core/control.c:2062:24: error: format string is not a string literal (potentially insecure) [-Werror,-Wformat-security] return request_module(module_name); ^~~~~~~~~~~
Use a string literal for the format string.
Link: https://github.com/ClangBuiltLinux/linux/issues/378 Signed-off-by: Bill Wendling isanbard@gmail.com --- sound/core/control.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/core/control.c b/sound/core/control.c index a25c0d64d104..a1778137147d 100644 --- a/sound/core/control.c +++ b/sound/core/control.c @@ -2059,7 +2059,7 @@ int snd_ctl_request_layer(const char *module_name) up_read(&snd_ctl_layer_rwsem); if (lops) return 0; - return request_module(module_name); + return request_module("%s", module_name); } EXPORT_SYMBOL_GPL(snd_ctl_request_layer);
From: Bill Wendling isanbard@gmail.com
When compiling with -Wformat, clang emits the following warnings:
net/netfilter/nf_conntrack_helper.c:168:18: error: format string is not a string literal (potentially insecure) [-Werror,-Wformat-security] request_module(mod_name); ^~~~~~~~
Use a string literal for the format string.
Link: https://github.com/ClangBuiltLinux/linux/issues/378 Signed-off-by: Bill Wendling isanbard@gmail.com --- net/netfilter/nf_conntrack_helper.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/netfilter/nf_conntrack_helper.c b/net/netfilter/nf_conntrack_helper.c index c12a87ebc3ee..1e0424d37abc 100644 --- a/net/netfilter/nf_conntrack_helper.c +++ b/net/netfilter/nf_conntrack_helper.c @@ -165,7 +165,7 @@ nf_nat_helper_try_module_get(const char *name, u16 l3num, u8 protonum) if (!nat) { snprintf(mod_name, sizeof(mod_name), "%s", h->nat_mod_name); rcu_read_unlock(); - request_module(mod_name); + request_module("%s", mod_name);
rcu_read_lock(); nat = nf_conntrack_nat_helper_find(mod_name);
On Thu, Jun 09, 2022 at 10:16:31PM +0000, Bill Wendling wrote:
From: Bill Wendling isanbard@gmail.com
When compiling with -Wformat, clang emits the following warnings:
net/netfilter/nf_conntrack_helper.c:168:18: error: format string is not a string literal (potentially insecure) [-Werror,-Wformat-security] request_module(mod_name); ^~~~~~~~
Use a string literal for the format string.
Applied this patch to nf-next, thanks
On Thu, 9 Jun 2022 22:16:19 +0000 Bill Wendling morbo@google.com wrote:
This patch set fixes some clang warnings when -Wformat is enabled.
tldr:
- printk(msg); + printk("%s", msg);
the only reason to make this change is where `msg' could contain a `%'. Generally, it came from userspace. Otherwise these changes are a useless consumer of runtime resources.
I think it would be better to quieten clang in some fashion.
On Thu, Jun 9, 2022 at 3:25 PM Andrew Morton akpm@linux-foundation.org wrote:
On Thu, 9 Jun 2022 22:16:19 +0000 Bill Wendling morbo@google.com wrote:
This patch set fixes some clang warnings when -Wformat is enabled.
tldr:
printk(msg);
printk("%s", msg);the only reason to make this change is where `msg' could contain a `%'. Generally, it came from userspace.
It helps kernel developers not accidentally to insert an unescaped '%' in their messages, potentially exposing their code to an attack vector.
Otherwise these changes are a useless consumer of runtime resources.
Calling a "printf" style function is already insanely expensive. :-) I understand that it's not okay blithely to increase runtime resources simply because it's already slow, but in this case it's worthwhile.
I think it would be better to quieten clang in some fashion.
The "printk" and similar functions all have the "__printf" attribute. I don't know of a modification to that attribute which can turn off this type of check.
-bw
On Friday 2022-06-10 00:49, Bill Wendling wrote:
On Thu, Jun 9, 2022 at 3:25 PM Andrew Morton akpm@linux-foundation.org wrote:
On Thu, 9 Jun 2022 22:16:19 +0000 Bill Wendling morbo@google.com wrote:
This patch set fixes some clang warnings when -Wformat is enabled.
tldr:
printk(msg);
printk("%s", msg);Otherwise these changes are a useless consumer of runtime resources.
Calling a "printf" style function is already insanely expensive. [...] The "printk" and similar functions all have the "__printf" attribute. I don't know of a modification to that attribute which can turn off this type of check.
Perhaps you can split vprintk_store in the middle (after the call to vsnprintf), and offer the second half as a function of its own (e.g. "puts"). Then the tldr could be
- printk(msg); + puts(msg);
On Thu, Jun 9, 2022 at 4:03 PM Jan Engelhardt jengelh@inai.de wrote:
On Friday 2022-06-10 00:49, Bill Wendling wrote:
On Thu, Jun 9, 2022 at 3:25 PM Andrew Morton akpm@linux-foundation.org wrote:
On Thu, 9 Jun 2022 22:16:19 +0000 Bill Wendling morbo@google.com wrote:
This patch set fixes some clang warnings when -Wformat is enabled.
tldr:
printk(msg);
printk("%s", msg);Otherwise these changes are a useless consumer of runtime resources.
Calling a "printf" style function is already insanely expensive. [...] The "printk" and similar functions all have the "__printf" attribute. I don't know of a modification to that attribute which can turn off this type of check.
Perhaps you can split vprintk_store in the middle (after the call to vsnprintf), and offer the second half as a function of its own (e.g. "puts"). Then the tldr could be
- printk(msg);
- puts(msg);
That might be a nice compromise. Andrew, what do you think?
-bw
On Thu, 9 Jun 2022 16:16:16 -0700 Bill Wendling morbo@google.com wrote:
On Thu, Jun 9, 2022 at 4:03 PM Jan Engelhardt jengelh@inai.de wrote:
On Friday 2022-06-10 00:49, Bill Wendling wrote:
On Thu, Jun 9, 2022 at 3:25 PM Andrew Morton akpm@linux-foundation.org wrote:
On Thu, 9 Jun 2022 22:16:19 +0000 Bill Wendling morbo@google.com wrote:
This patch set fixes some clang warnings when -Wformat is enabled.
tldr:
printk(msg);
printk("%s", msg);Otherwise these changes are a useless consumer of runtime resources.
Calling a "printf" style function is already insanely expensive. [...] The "printk" and similar functions all have the "__printf" attribute. I don't know of a modification to that attribute which can turn off this type of check.
Perhaps you can split vprintk_store in the middle (after the call to vsnprintf), and offer the second half as a function of its own (e.g. "puts"). Then the tldr could be
- printk(msg);
- puts(msg);
That might be a nice compromise. Andrew, what do you think?
Sure. It's surprising that we don't already have a puts().
On Thu, Jun 09, 2022 at 04:16:16PM -0700, Bill Wendling wrote:
On Thu, Jun 9, 2022 at 4:03 PM Jan Engelhardt jengelh@inai.de wrote:
On Friday 2022-06-10 00:49, Bill Wendling wrote:
On Thu, Jun 9, 2022 at 3:25 PM Andrew Morton akpm@linux-foundation.org wrote:
On Thu, 9 Jun 2022 22:16:19 +0000 Bill Wendling morbo@google.com wrote:
This patch set fixes some clang warnings when -Wformat is enabled.
tldr:
printk(msg);
printk("%s", msg);Otherwise these changes are a useless consumer of runtime resources.
Calling a "printf" style function is already insanely expensive. [...] The "printk" and similar functions all have the "__printf" attribute. I don't know of a modification to that attribute which can turn off this type of check.
Perhaps you can split vprintk_store in the middle (after the call to vsnprintf), and offer the second half as a function of its own (e.g. "puts"). Then the tldr could be
- printk(msg);
- puts(msg);
That might be a nice compromise. Andrew, what do you think?
You would need to do that for all of the dev_printk() variants, so I doubt that would ever be all that useful as almost no one should be using a "raw" printk() these days.
thanks,
greg k-h
On Fri, 2022-06-10 at 07:20 +0200, Greg Kroah-Hartman wrote:
On Thu, Jun 09, 2022 at 04:16:16PM -0700, Bill Wendling wrote:
On Thu, Jun 9, 2022 at 4:03 PM Jan Engelhardt jengelh@inai.de wrote:
On Friday 2022-06-10 00:49, Bill Wendling wrote:
On Thu, Jun 9, 2022 at 3:25 PM Andrew Morton akpm@linux-foundation.org wrote:
On Thu, 9 Jun 2022 22:16:19 +0000 Bill Wendling morbo@google.com wrote:
This patch set fixes some clang warnings when -Wformat is enabled.
tldr:
printk(msg);
printk("%s", msg);Otherwise these changes are a useless consumer of runtime resources.
Calling a "printf" style function is already insanely expensive.
I expect the printk code itself dominates, not the % scan cost.
Perhaps you can split vprintk_store in the middle (after the call to vsnprintf), and offer the second half as a function of its own (e.g. "puts"). Then the tldr could be
- printk(msg);
- puts(msg);
That might be a nice compromise. Andrew, what do you think?
You would need to do that for all of the dev_printk() variants, so I doubt that would ever be all that useful as almost no one should be using a "raw" printk() these days.
True. The kernel has ~20K variants like that.
$ git grep -P '\b(?:(?:\w+_){1,3}(?:alert|emerg|crit|err|warn|notice|info|cont|debug|dbg)|printk)\s*(".*"\s*)\s*;' | wc -l 21160
That doesn't include the ~3K uses like
#define foo "bar" printk(foo);
$ git grep -P '\b(?:(?:\w+_){1,3}(?:alert|emerg|crit|err|warn|info|notice|debug|dbg|cont)|printk)\s*((?:\s*\w+){1,3}\s*)\s*;'|wc -l 2922
There are apparently only a few hundred uses of variants like:
printk("%s", foo)
$ git grep -P '\b(?:(?:\w+_){1,3}(?:alert|emerg|crit|err|warn|info|notice|debug|dbg|cont)|printk)\s*(\s*"%s(?:\n)?"\s*,\s*(?:".*"|\w+)\s*)\s*;' | wc -l 305
unless I screwed up my greps (which of course is quite possible)
From: Bill Wendling
Sent: 09 June 2022 23:49
On Thu, Jun 9, 2022 at 3:25 PM Andrew Morton akpm@linux-foundation.org wrote:
On Thu, 9 Jun 2022 22:16:19 +0000 Bill Wendling morbo@google.com wrote:
This patch set fixes some clang warnings when -Wformat is enabled.
tldr:
printk(msg);
printk("%s", msg);the only reason to make this change is where `msg' could contain a `%'. Generally, it came from userspace.
It helps kernel developers not accidentally to insert an unescaped '%' in their messages, potentially exposing their code to an attack vector.
Otherwise these changes are a useless consumer of runtime resources.
Calling a "printf" style function is already insanely expensive. :-) I understand that it's not okay blithely to increase runtime resources simply because it's already slow, but in this case it's worthwhile.
Yep, IMHO definitely should be fixed. It is even possible that using "%s" is faster because the printf code doesn't have to scan the string for format effectors.
I think it would be better to quieten clang in some fashion.
The "printk" and similar functions all have the "__printf" attribute. I don't know of a modification to that attribute which can turn off this type of check.
And you wouldn't want to for these cases.
The only problems arise when the format is calculated (or passed in from a caller). But that is likely to be dangerous - reading formats from files (eg for language translation) isn't a good idea at all.
David
- Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On Friday 2022-06-10 10:17, David Laight wrote:
Calling a "printf" style function is already insanely expensive. :-) I understand that it's not okay blithely to increase runtime resources simply because it's already slow, but in this case it's worthwhile.
Yep, IMHO definitely should be fixed. It is even possible that using "%s" is faster because the printf code doesn't have to scan the string for format effectors.
I see no special handling; the vsnprintf function just loops over fmt as usual and I see no special casing of fmt by e.g. strcmp(fmt, "%s") == 0 to take a shortcut.
From: Jan Engelhardt
Sent: 10 June 2022 09:32
On Friday 2022-06-10 10:17, David Laight wrote:
Calling a "printf" style function is already insanely expensive. :-) I understand that it's not okay blithely to increase runtime resources simply because it's already slow, but in this case it's worthwhile.
Yep, IMHO definitely should be fixed. It is even possible that using "%s" is faster because the printf code doesn't have to scan the string for format effectors.
I see no special handling; the vsnprintf function just loops over fmt as usual and I see no special casing of fmt by e.g. strcmp(fmt, "%s") == 0 to take a shortcut.
Consider the difference between: printf("fubar"); and printf("%s", "fubar"); In the former all of "fubar" is checked for '%'. In the latter only the length of "fubar" has to be counted.
FWIW the patch description should probably by: use "%s" when formatting a single string. (or something to that effect).
David
- Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On Friday 2022-06-10 11:14, David Laight wrote:
Yep, IMHO definitely should be fixed. It is even possible that using "%s" is faster because the printf code doesn't have to scan the string for format effectors.
I see no special handling; the vsnprintf function just loops over fmt as usual and I see no special casing of fmt by e.g. strcmp(fmt, "%s") == 0 to take a shortcut.
Consider the difference between: printf("fubar"); and printf("%s", "fubar"); In the former all of "fubar" is checked for '%'. In the latter only the length of "fubar" has to be counted.
To check the length of "fubar", printf first needs to know that there even is an argument to be pulled from the stack, which it does by evaluating the format string.
So, in fairness, it's more like:
In the latter, all of "%s" is checked for '%'.
On Thu, Jun 9, 2022 at 3:25 PM Andrew Morton akpm@linux-foundation.org wrote:
On Thu, 9 Jun 2022 22:16:19 +0000 Bill Wendling morbo@google.com wrote:
This patch set fixes some clang warnings when -Wformat is enabled.
It looks like this series fixes -Wformat-security, which while being a member of the -Wformat group, is intentionally disabled in the kernel and somewhat orthogonal to enabling -Wformat with Clang.
-Wformat is a group flag (like -Wall) that enables multiple other flags implicitly. Reading through clang/include/clang/Basic/DiagnosticGroups.td in clang's sources, it looks like:
1. -Wformat is a group flag. 2. -Wformat-security is a member of the -Wformat group; enabling -Wformat will enable -Wformat-security. 3. -Wformat itself is a member of -Wmost (never heard of -Wmost, but w/e). So -Wmost will enable -Wformat will enable -Wformat-security. 4. -Wmost is itself a member of -Wall. -Wall enables -Wmost enables -Wformat enables -Wformat security.
Looking now at Kbuild: 1. Makefile:523 adds -Wall to KBUILD_CFLAGS. 2. The same assignment expression but on line 526 immediately disables -Wformat-security via -Wno-format-security. 3. scripts/Makefile.extrawarn disables -Wformat via -Wno-format only for clang (via guard of CONFIG_CC_IS_CLANG).
We _want_ -Wformat enabled for clang so that developers aren't sending patches that trigger -Wformat with GCC (if they didn't happen to test their code with both). It's disabled for clang until we can build the kernel cleanly with it enabled, which we'd like to do.
I don't think that we need to enable -Wformat-security to build with -Wformat for clang.
I suspect based on Randy's comment on patch 1/12 that perhaps -Wformat was _added_ to KBUILD_CFLAGS in scripts/Makefile.extrawarn rather than -Wno-format being _removed_. The former would re-enable -Wformat-security due to the grouping logic described above. The latter is probably closer to our ultimate goal of enabling -Wformat coverage for clang (or rather not disabling the coverage via -Wno-format; a double negative).
I'm pretty sure the kernel doesn't support %n in format strings...see the comment above vsnprintf in lib/vsprintf.c. Are there other attacks other than %n that -Wformat-security guards against? Maybe there's some context on the commit that added -Wno-format-security to the kernel? Regardless, I don't think enabling -Wformat-security is a blocker for enabling -Wformat (or...disabling -Wno-format...two sides of the same coin) for clang.
tldr:
printk(msg);
printk("%s", msg);the only reason to make this change is where `msg' could contain a `%'. Generally, it came from userspace. Otherwise these changes are a useless consumer of runtime resources.
I think it would be better to quieten clang in some fashion.
participants (11)
-
Andrew Morton -
Bill Wendling -
Christoph Hellwig -
David Laight -
Greg Kroah-Hartman -
Jan Engelhardt -
Joe Perches -
Nick Desaulniers -
Pablo Neira Ayuso -
Phillip Potter -
Randy Dunlap