| Message ID | 20220609221702.347522-8-morbo@google.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | Clang -Wformat warning fixes | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Guessed tree name to be net-next |
| netdev/fixes_present | success | Fixes tag not required for -next series |
| netdev/subject_prefix | success | Link |
| netdev/cover_letter | success | Series has a cover letter |
| netdev/patch_count | success | Link |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | success | Errors and warnings before: 2 this patch: 2 |
| netdev/cc_maintainers | success | CCed 6 of 6 maintainers |
| netdev/build_clang | success | Errors and warnings before: 6 this patch: 6 |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/check_selftest | success | No net selftest shell script |
| netdev/verify_fixes | success | No Fixes tag |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 2 this patch: 2 |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 8 lines checked |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
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
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();