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(-)
--
2.36.1.255.ge46751e96f-goog
From: Bill Wendling <[email protected]>
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 <[email protected]>
---
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);
--
2.36.1.255.ge46751e96f-goog
From: Bill Wendling <[email protected]>
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 <[email protected]>
---
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;
--
2.36.1.255.ge46751e96f-goog
From: Bill Wendling <[email protected]>
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 <[email protected]>
---
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);
--
2.36.1.255.ge46751e96f-goog
From: Bill Wendling <[email protected]>
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 <[email protected]>
---
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 */
--
2.36.1.255.ge46751e96f-goog
From: Bill Wendling <[email protected]>
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 <[email protected]>
---
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);
--
2.36.1.255.ge46751e96f-goog
From: Bill Wendling <[email protected]>
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 <[email protected]>
---
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");
--
2.36.1.255.ge46751e96f-goog
From: Bill Wendling <[email protected]>
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 <[email protected]>
---
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();
--
2.36.1.255.ge46751e96f-goog
From: Bill Wendling <[email protected]>
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 <[email protected]>
---
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);
--
2.36.1.255.ge46751e96f-goog
From: Bill Wendling <[email protected]>
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 <[email protected]>
---
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);
--
2.36.1.255.ge46751e96f-goog
From: Bill Wendling <[email protected]>
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 <[email protected]>
---
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;
--
2.36.1.255.ge46751e96f-goog
From: Bill Wendling <[email protected]>
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 <[email protected]>
---
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);
--
2.36.1.255.ge46751e96f-goog
On Thu, 9 Jun 2022 22:16:19 +0000 Bill Wendling <[email protected]> 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.
From: Bill Wendling <[email protected]>
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 <[email protected]>
---
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
On Friday 2022-06-10 00:49, Bill Wendling wrote:
>On Thu, Jun 9, 2022 at 3:25 PM Andrew Morton <[email protected]> wrote:
>> On Thu, 9 Jun 2022 22:16:19 +0000 Bill Wendling <[email protected]> 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 3:25 PM Andrew Morton <[email protected]> wrote:
>
> On Thu, 9 Jun 2022 22:16:19 +0000 Bill Wendling <[email protected]> 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 Thu, Jun 9, 2022 at 4:15 PM Randy Dunlap <[email protected]> wrote:
> On 6/9/22 15:16, Bill Wendling wrote:
> > From: Bill Wendling <[email protected]>
> >
> > 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 <[email protected]>
> > ---
> > 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
On 6/9/22 15:16, Bill Wendling wrote:
> From: Bill Wendling <[email protected]>
>
> 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 <[email protected]>
> ---
> 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);
> } else
> pr_emerg(HW_ERR "Fake kernel panic: %s\n", msg);
>
--
~Randy
On Thu, Jun 9, 2022 at 4:03 PM Jan Engelhardt <[email protected]> wrote:
> On Friday 2022-06-10 00:49, Bill Wendling wrote:
> >On Thu, Jun 9, 2022 at 3:25 PM Andrew Morton <[email protected]> wrote:
> >> On Thu, 9 Jun 2022 22:16:19 +0000 Bill Wendling <[email protected]> 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, Jun 9, 2022 at 3:25 PM Andrew Morton <[email protected]> wrote:
>
> On Thu, 9 Jun 2022 22:16:19 +0000 Bill Wendling <[email protected]> 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.
--
Thanks,
~Nick Desaulniers
On Thu, 9 Jun 2022 16:16:16 -0700 Bill Wendling <[email protected]> wrote:
> On Thu, Jun 9, 2022 at 4:03 PM Jan Engelhardt <[email protected]> wrote:
> > On Friday 2022-06-10 00:49, Bill Wendling wrote:
> > >On Thu, Jun 9, 2022 at 3:25 PM Andrew Morton <[email protected]> wrote:
> > >> On Thu, 9 Jun 2022 22:16:19 +0000 Bill Wendling <[email protected]> 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 <[email protected]> wrote:
> > On Friday 2022-06-10 00:49, Bill Wendling wrote:
> > >On Thu, Jun 9, 2022 at 3:25 PM Andrew Morton <[email protected]> wrote:
> > >> On Thu, 9 Jun 2022 22:16:19 +0000 Bill Wendling <[email protected]> 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 Thu, Jun 09, 2022 at 10:16:26PM +0000, Bill Wendling wrote:
> From: Bill Wendling <[email protected]>
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 <[email protected]>
> ---
> 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 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
> Sent: 09 June 2022 23:49
>
> On Thu, Jun 9, 2022 at 3:25 PM Andrew Morton <[email protected]> wrote:
> >
> > On Thu, 9 Jun 2022 22:16:19 +0000 Bill Wendling <[email protected]> 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.
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 '%'.
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 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 <[email protected]> wrote:
> > > On Friday 2022-06-10 00:49, Bill Wendling wrote:
> > > > On Thu, Jun 9, 2022 at 3:25 PM Andrew Morton <[email protected]> wrote:
> > > > > On Thu, 9 Jun 2022 22:16:19 +0000 Bill Wendling <[email protected]> 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)
On Thu, Jun 09, 2022 at 10:16:27PM +0000, Bill Wendling wrote:
> From: Bill Wendling <[email protected]>
>
> 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 <[email protected]>
> ---
> 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 Thu, Jun 9, 2022 at 10:18 PM Greg Kroah-Hartman
<[email protected]> wrote:
>
> On Thu, Jun 09, 2022 at 10:16:26PM +0000, Bill Wendling wrote:
> > From: Bill Wendling <[email protected]>
>
> 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 <[email protected]>
> > ---
> > 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
On Sun, Jun 12, 2022 at 9:23 AM Phillip Potter <[email protected]> wrote:
>
> On Thu, Jun 09, 2022 at 10:16:27PM +0000, Bill Wendling wrote:
> > From: Bill Wendling <[email protected]>
> >
> > 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 <[email protected]>
> > ---
> > 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
On Thu, Jun 09, 2022 at 10:16:31PM +0000, Bill Wendling wrote:
> From: Bill Wendling <[email protected]>
>
> 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