* [PATCH v2 1/2] kgdboc: Passing ekgdboc to command line causes panic @ 2018-08-14 12:46 zhe.he 2018-08-14 12:46 ` [PATCH v2 2/2] kgdboc: Change printk to the right fashion zhe.he 2018-08-14 13:26 ` [PATCH v2 1/2] kgdboc: Passing ekgdboc to command line causes panic Daniel Thompson 0 siblings, 2 replies; 7+ messages in thread From: zhe.he @ 2018-08-14 12:46 UTC (permalink / raw) To: jason.wessel, daniel.thompson, gregkh, jslaby, kgdb-bugreport, linux-serial, linux-kernel Cc: zhe.he From: He Zhe <zhe.he@windriver.com> kgdboc_option_setup does not check input argument before passing it to strlen. The argument would be a NULL pointer if "ekgdboc", without its value, is set in command line and thus cause the following panic. PANIC: early exception 0xe3 IP 10:ffffffff8fbbb620 error 0 cr2 0x0 [ 0.000000] CPU: 0 PID: 0 Comm: swapper Not tainted 4.18-rc8+ #1 [ 0.000000] RIP: 0010:strlen+0x0/0x20 ... [ 0.000000] Call Trace [ 0.000000] ? kgdboc_option_setup+0x9/0xa0 [ 0.000000] ? kgdboc_early_init+0x6/0x1b [ 0.000000] ? do_early_param+0x4d/0x82 [ 0.000000] ? parse_args+0x212/0x330 [ 0.000000] ? rdinit_setup+0x26/0x26 [ 0.000000] ? parse_early_options+0x20/0x23 [ 0.000000] ? rdinit_setup+0x26/0x26 [ 0.000000] ? parse_early_param+0x2d/0x39 [ 0.000000] ? setup_arch+0x2f7/0xbf4 [ 0.000000] ? start_kernel+0x5e/0x4c2 [ 0.000000] ? load_ucode_bsp+0x113/0x12f [ 0.000000] ? secondary_startup_64+0xa5/0xb0 This patch adds a check to prevent the panic. Cc: stable@vger.kernel.org Signed-off-by: He Zhe <zhe.he@windriver.com> --- v2: - Split out printk cleanups - Add cc to stable@vger.kernel.org drivers/tty/serial/kgdboc.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/drivers/tty/serial/kgdboc.c b/drivers/tty/serial/kgdboc.c index b4ba2b1..206f8c2 100644 --- a/drivers/tty/serial/kgdboc.c +++ b/drivers/tty/serial/kgdboc.c @@ -130,6 +130,11 @@ static void kgdboc_unregister_kbd(void) static int kgdboc_option_setup(char *opt) { + if (!opt) { + pr_err("kgdboc: null option\n"); + return -EINVAL; + } + if (strlen(opt) >= MAX_CONFIG_LEN) { printk(KERN_ERR "kgdboc: config string too long\n"); return -ENOSPC; -- 2.7.4 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH v2 2/2] kgdboc: Change printk to the right fashion 2018-08-14 12:46 [PATCH v2 1/2] kgdboc: Passing ekgdboc to command line causes panic zhe.he @ 2018-08-14 12:46 ` zhe.he 2018-08-14 13:35 ` Daniel Thompson 2018-08-14 13:26 ` [PATCH v2 1/2] kgdboc: Passing ekgdboc to command line causes panic Daniel Thompson 1 sibling, 1 reply; 7+ messages in thread From: zhe.he @ 2018-08-14 12:46 UTC (permalink / raw) To: jason.wessel, daniel.thompson, gregkh, jslaby, kgdb-bugreport, linux-serial, linux-kernel Cc: zhe.he From: He Zhe <zhe.he@windriver.com> pr_* is preferred according to scripts/checkpatch.pl. Cc: stable@vger.kernel.org Signed-off-by: He Zhe <zhe.he@windriver.com> --- v2: - Split printk cleanups into a single patch - Add cc to stable@vger.kernel.org drivers/tty/serial/kgdboc.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/drivers/tty/serial/kgdboc.c b/drivers/tty/serial/kgdboc.c index 206f8c2..0003d6c 100644 --- a/drivers/tty/serial/kgdboc.c +++ b/drivers/tty/serial/kgdboc.c @@ -136,7 +136,7 @@ static int kgdboc_option_setup(char *opt) } if (strlen(opt) >= MAX_CONFIG_LEN) { - printk(KERN_ERR "kgdboc: config string too long\n"); + pr_err("kgdboc: config string too long\n"); return -ENOSPC; } strcpy(config, opt); @@ -253,7 +253,7 @@ static int param_set_kgdboc_var(const char *kmessage, int len = strlen(kmessage); if (len >= MAX_CONFIG_LEN) { - printk(KERN_ERR "kgdboc: config string too long\n"); + pr_err("kgdboc: config string too long\n"); return -ENOSPC; } @@ -264,8 +264,7 @@ static int param_set_kgdboc_var(const char *kmessage, } if (kgdb_connected) { - printk(KERN_ERR - "kgdboc: Cannot reconfigure while KGDB is connected.\n"); + pr_err("kgdboc: Cannot reconfigure while KGDB is connected.\n"); return -EBUSY; } -- 2.7.4 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2 2/2] kgdboc: Change printk to the right fashion 2018-08-14 12:46 ` [PATCH v2 2/2] kgdboc: Change printk to the right fashion zhe.he @ 2018-08-14 13:35 ` Daniel Thompson 2018-08-14 14:04 ` Joe Perches 0 siblings, 1 reply; 7+ messages in thread From: Daniel Thompson @ 2018-08-14 13:35 UTC (permalink / raw) To: zhe.he Cc: jason.wessel, gregkh, jslaby, kgdb-bugreport, linux-serial, linux-kernel On Tue, Aug 14, 2018 at 08:46:01PM +0800, zhe.he@windriver.com wrote: > From: He Zhe <zhe.he@windriver.com> > > pr_* is preferred according to scripts/checkpatch.pl. > > Cc: stable@vger.kernel.org This change does not "fix a problem that causes a build error (but not for things marked CONFIG_BROKEN), an oops, a hang, data corruption, a real security issue, or some “oh, that’s not good” issue. In short, something critical.". Only the first patch meets this criteria and only that patch should be Cc:ed to stable@ . Please remove from this patch. > Signed-off-by: He Zhe <zhe.he@windriver.com> > --- > v2: > - Split printk cleanups into a single patch > - Add cc to stable@vger.kernel.org > > drivers/tty/serial/kgdboc.c | 7 +++---- > 1 file changed, 3 insertions(+), 4 deletions(-) > > diff --git a/drivers/tty/serial/kgdboc.c b/drivers/tty/serial/kgdboc.c > index 206f8c2..0003d6c 100644 > --- a/drivers/tty/serial/kgdboc.c > +++ b/drivers/tty/serial/kgdboc.c > @@ -136,7 +136,7 @@ static int kgdboc_option_setup(char *opt) > } > > if (strlen(opt) >= MAX_CONFIG_LEN) { > - printk(KERN_ERR "kgdboc: config string too long\n"); > + pr_err("kgdboc: config string too long\n"); Looks like you should remove the tags from pr_err and use pr_fmt(fmt) to put tags on the messages: #define pr_fmt(fmt) "kgdboc: " fmt Daniel. > return -ENOSPC; > } > strcpy(config, opt); > @@ -253,7 +253,7 @@ static int param_set_kgdboc_var(const char *kmessage, > int len = strlen(kmessage); > > if (len >= MAX_CONFIG_LEN) { > - printk(KERN_ERR "kgdboc: config string too long\n"); > + pr_err("kgdboc: config string too long\n"); > return -ENOSPC; > } > > @@ -264,8 +264,7 @@ static int param_set_kgdboc_var(const char *kmessage, > } > > if (kgdb_connected) { > - printk(KERN_ERR > - "kgdboc: Cannot reconfigure while KGDB is connected.\n"); > + pr_err("kgdboc: Cannot reconfigure while KGDB is connected.\n"); > > return -EBUSY; > } > -- > 2.7.4 > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2 2/2] kgdboc: Change printk to the right fashion 2018-08-14 13:35 ` Daniel Thompson @ 2018-08-14 14:04 ` Joe Perches 2018-08-14 14:41 ` Daniel Thompson 0 siblings, 1 reply; 7+ messages in thread From: Joe Perches @ 2018-08-14 14:04 UTC (permalink / raw) To: Daniel Thompson, zhe.he Cc: jason.wessel, gregkh, jslaby, kgdb-bugreport, linux-serial, linux-kernel On Tue, 2018-08-14 at 14:35 +0100, Daniel Thompson wrote: > On Tue, Aug 14, 2018 at 08:46:01PM +0800, zhe.he@windriver.com wrote: > > From: He Zhe <zhe.he@windriver.com> > > > > pr_* is preferred according to scripts/checkpatch.pl. [] > > diff --git a/drivers/tty/serial/kgdboc.c b/drivers/tty/serial/kgdboc.c [] > > @@ -136,7 +136,7 @@ static int kgdboc_option_setup(char *opt) > > } > > > > if (strlen(opt) >= MAX_CONFIG_LEN) { > > - printk(KERN_ERR "kgdboc: config string too long\n"); > > + pr_err("kgdboc: config string too long\n"); > > Looks like you should remove the tags from pr_err and use pr_fmt(fmt) to > put tags on the messages: > > #define pr_fmt(fmt) "kgdboc: " fmt True and it's probably better to use: #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2 2/2] kgdboc: Change printk to the right fashion 2018-08-14 14:04 ` Joe Perches @ 2018-08-14 14:41 ` Daniel Thompson 2018-08-14 15:39 ` He Zhe 0 siblings, 1 reply; 7+ messages in thread From: Daniel Thompson @ 2018-08-14 14:41 UTC (permalink / raw) To: Joe Perches Cc: zhe.he, jason.wessel, gregkh, jslaby, kgdb-bugreport, linux-serial, linux-kernel On Tue, Aug 14, 2018 at 07:04:11AM -0700, Joe Perches wrote: > On Tue, 2018-08-14 at 14:35 +0100, Daniel Thompson wrote: > > On Tue, Aug 14, 2018 at 08:46:01PM +0800, zhe.he@windriver.com wrote: > > > From: He Zhe <zhe.he@windriver.com> > > > > > > pr_* is preferred according to scripts/checkpatch.pl. > [] > > > diff --git a/drivers/tty/serial/kgdboc.c b/drivers/tty/serial/kgdboc.c > [] > > > @@ -136,7 +136,7 @@ static int kgdboc_option_setup(char *opt) > > > } > > > > > > if (strlen(opt) >= MAX_CONFIG_LEN) { > > > - printk(KERN_ERR "kgdboc: config string too long\n"); > > > + pr_err("kgdboc: config string too long\n"); > > > > Looks like you should remove the tags from pr_err and use pr_fmt(fmt) to > > put tags on the messages: > > > > #define pr_fmt(fmt) "kgdboc: " fmt > > True and it's probably better to use: > > #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt Good point! Thanks. Daniel. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2 2/2] kgdboc: Change printk to the right fashion 2018-08-14 14:41 ` Daniel Thompson @ 2018-08-14 15:39 ` He Zhe 0 siblings, 0 replies; 7+ messages in thread From: He Zhe @ 2018-08-14 15:39 UTC (permalink / raw) To: Daniel Thompson, Joe Perches Cc: jason.wessel, gregkh, jslaby, kgdb-bugreport, linux-serial, linux-kernel On 2018年08月14日 22:41, Daniel Thompson wrote: > On Tue, Aug 14, 2018 at 07:04:11AM -0700, Joe Perches wrote: >> On Tue, 2018-08-14 at 14:35 +0100, Daniel Thompson wrote: >>> On Tue, Aug 14, 2018 at 08:46:01PM +0800, zhe.he@windriver.com wrote: >>>> From: He Zhe <zhe.he@windriver.com> >>>> >>>> pr_* is preferred according to scripts/checkpatch.pl. >> [] >>>> diff --git a/drivers/tty/serial/kgdboc.c b/drivers/tty/serial/kgdboc.c >> [] >>>> @@ -136,7 +136,7 @@ static int kgdboc_option_setup(char *opt) >>>> } >>>> >>>> if (strlen(opt) >= MAX_CONFIG_LEN) { >>>> - printk(KERN_ERR "kgdboc: config string too long\n"); >>>> + pr_err("kgdboc: config string too long\n"); >>> Looks like you should remove the tags from pr_err and use pr_fmt(fmt) to >>> put tags on the messages: >>> >>> #define pr_fmt(fmt) "kgdboc: " fmt >> True and it's probably better to use: >> >> #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt > Good point! Thanks. Daniel, Joe, thanks for your good suggestions. v3 is sent. Zhe > > > Daniel. > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2 1/2] kgdboc: Passing ekgdboc to command line causes panic 2018-08-14 12:46 [PATCH v2 1/2] kgdboc: Passing ekgdboc to command line causes panic zhe.he 2018-08-14 12:46 ` [PATCH v2 2/2] kgdboc: Change printk to the right fashion zhe.he @ 2018-08-14 13:26 ` Daniel Thompson 1 sibling, 0 replies; 7+ messages in thread From: Daniel Thompson @ 2018-08-14 13:26 UTC (permalink / raw) To: zhe.he Cc: jason.wessel, gregkh, jslaby, kgdb-bugreport, linux-serial, linux-kernel On Tue, Aug 14, 2018 at 08:46:00PM +0800, zhe.he@windriver.com wrote: > From: He Zhe <zhe.he@windriver.com> > > kgdboc_option_setup does not check input argument before passing it > to strlen. The argument would be a NULL pointer if "ekgdboc", without > its value, is set in command line and thus cause the following panic. > > PANIC: early exception 0xe3 IP 10:ffffffff8fbbb620 error 0 cr2 0x0 > [ 0.000000] CPU: 0 PID: 0 Comm: swapper Not tainted 4.18-rc8+ #1 > [ 0.000000] RIP: 0010:strlen+0x0/0x20 > ... > [ 0.000000] Call Trace > [ 0.000000] ? kgdboc_option_setup+0x9/0xa0 > [ 0.000000] ? kgdboc_early_init+0x6/0x1b > [ 0.000000] ? do_early_param+0x4d/0x82 > [ 0.000000] ? parse_args+0x212/0x330 > [ 0.000000] ? rdinit_setup+0x26/0x26 > [ 0.000000] ? parse_early_options+0x20/0x23 > [ 0.000000] ? rdinit_setup+0x26/0x26 > [ 0.000000] ? parse_early_param+0x2d/0x39 > [ 0.000000] ? setup_arch+0x2f7/0xbf4 > [ 0.000000] ? start_kernel+0x5e/0x4c2 > [ 0.000000] ? load_ucode_bsp+0x113/0x12f > [ 0.000000] ? secondary_startup_64+0xa5/0xb0 > > This patch adds a check to prevent the panic. > > Cc: stable@vger.kernel.org > Signed-off-by: He Zhe <zhe.he@windriver.com> > --- > v2: > - Split out printk cleanups > - Add cc to stable@vger.kernel.org > > drivers/tty/serial/kgdboc.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/drivers/tty/serial/kgdboc.c b/drivers/tty/serial/kgdboc.c > index b4ba2b1..206f8c2 100644 > --- a/drivers/tty/serial/kgdboc.c > +++ b/drivers/tty/serial/kgdboc.c > @@ -130,6 +130,11 @@ static void kgdboc_unregister_kbd(void) > > static int kgdboc_option_setup(char *opt) > { > + if (!opt) { > + pr_err("kgdboc: null option\n"); Apologies... I should have picked this up when I replied earlier but this error message describes what the function gets when I think it should report what the user actually did. So should be something like: pr_err("kgdboc: config string not provided\n"); With that (or a very similar) change: Reviewed-by: Daniel Thompson <daniel.thompson@linaro.org> Daniel. > + return -EINVAL; > + } > + > if (strlen(opt) >= MAX_CONFIG_LEN) { > printk(KERN_ERR "kgdboc: config string too long\n"); > return -ENOSPC; > -- > 2.7.4 > ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2018-08-14 15:39 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-08-14 12:46 [PATCH v2 1/2] kgdboc: Passing ekgdboc to command line causes panic zhe.he 2018-08-14 12:46 ` [PATCH v2 2/2] kgdboc: Change printk to the right fashion zhe.he 2018-08-14 13:35 ` Daniel Thompson 2018-08-14 14:04 ` Joe Perches 2018-08-14 14:41 ` Daniel Thompson 2018-08-14 15:39 ` He Zhe 2018-08-14 13:26 ` [PATCH v2 1/2] kgdboc: Passing ekgdboc to command line causes panic Daniel Thompson
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).