[PATCH] kgdbts: Passing ekgdbts to command line causes panic

[PATCH] kgdbts: Passing ekgdbts to command line causes panic

[Defang Bo]

Similar to commit<1bd54d85>,kgdbts_option_setup does not check input argument
before passing it to strlen. The argument would be a NULL pointer.

Signed-off-by: Defang Bo <bodefang@126.com>
---
 drivers/misc/kgdbts.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/misc/kgdbts.c b/drivers/misc/kgdbts.c
index 945701b..99d9795 100644
--- a/drivers/misc/kgdbts.c
+++ b/drivers/misc/kgdbts.c
@@ -1057,6 +1057,11 @@ static void kgdbts_run_tests(void)
 
 static int kgdbts_option_setup(char *opt)
 {
+	if (!opt) {
+		pr_err("kgdbts: config string not provided\n");
+		return -EINVAL;
+	}
+
 	if (strlen(opt) >= MAX_CONFIG_LEN) {
 		printk(KERN_ERR "kgdbts: config string too long\n");
 		return -ENOSPC;
-- 
2.7.4

Re: [PATCH] kgdbts: Passing ekgdbts to command line causes panic

[Greg KH]

On Sun, Dec 27, 2020 at 09:43:05PM +0800, Defang Bo wrote:
> Similar to commit<1bd54d85>,kgdbts_option_setup does not check input argument
> before passing it to strlen. The argument would be a NULL pointer.
> 

As per the documentation, you should write commits out as 1bd54d851f50
("kgdboc: Passing ekgdboc to command line causes panic").

Can you fix that up and resend this?

thanks,

greg k-h

Re: [PATCH] kgdbts: Passing ekgdbts to command line causes panic

[Greg KH]

On Mon, Dec 28, 2020 at 09:58:58AM +0800, Defang Bo wrote:
> Similar to commit<1bd54d851f50>("kgdboc: Passing ekgdboc to command line causes panic"),

Please wrap your lines, and use the correct way to reference a commit in
a changelog as documented.  It should look like:
	1bd54d851f50 ("kgdboc: Passing ekgdboc to command line causes panic")

> kgdbts_option_setup does not check input argument before passing it to strlen.
> The argument would be a NULL pointer.

What argument?

As others pointed out, your subject line does not make sense, please fix
up.

thanks,

greg k-h

Re: [PATCH] kgdbts: Passing ekgdbts to command line causes panic

[Daniel Thompson]

Please avoid top posting. Threads on LKML are typically presented as
follows.

On Tue, Jan 05, 2021 at 11:21:38AM +0800, bodefang wrote:
> At 2021-01-04 19:28:54, "Daniel Thompson" <daniel.thompson@linaro.org>
> wrote:
> >On Mon, Dec 28, 2020 at 09:58:58AM +0800, Defang Bo wrote:
> >> Similar to commit<1bd54d851f50>("kgdboc: Passing ekgdboc to command
> >> line causes panic"), kgdbts_option_setup does not check input
> >> argument before passing it to strlen.  The argument would be a NULL
> >> pointer.
> >
> > Something seems to be missing here.
> >
> > The ekgdbts parameter mentioned in the subject line doesn't exist so
> > how can including it on the kernel command line could provoke a
> > panic.
> >
> > Please can you share the kernel boot arguments you used when you
> > tested this patch?
>
> I use static analysis tool to find these funcs are similar to the
> commit<1bd54d851f50>(kgdboc: Passing ekgdboc to command line causes
> panic), so it's just defensive, I haven't actually hitted this but
> there actually seems a problem here.

I don't see how a problem that occured when ekgdboc is parsed can occur
because this module does not parse ekgdbts! Are there really any
circumstances where kgdbts_option_setup() can be called with a NULL
argument?


Daniel.

Re: [PATCH] kgdbts: Passing ekgdbts to command line causes panic

[Daniel Thompson]

On Mon, Dec 28, 2020 at 09:58:58AM +0800, Defang Bo wrote:
> Similar to commit<1bd54d851f50>("kgdboc: Passing ekgdboc to command line causes panic"),
> kgdbts_option_setup does not check input argument before passing it to strlen.
> The argument would be a NULL pointer.

Something seems to be missing here.

The ekgdbts parameter mentioned in the subject line doesn't exist so
how can including it on the kernel command line could provoke a panic.

Please can you share the kernel boot arguments you used when you tested
this patch?


Daniel.


> Signed-off-by: Defang Bo <bodefang@126.com>
> ---
>  drivers/misc/kgdbts.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/misc/kgdbts.c b/drivers/misc/kgdbts.c
> index 945701b..b077547 100644
> --- a/drivers/misc/kgdbts.c
> +++ b/drivers/misc/kgdbts.c
> @@ -1057,6 +1057,11 @@ static void kgdbts_run_tests(void)
>  
>  static int kgdbts_option_setup(char *opt)
>  {
> +	if (!opt) {
> +		pr_err("kgdbts: config string not provided\n");
> +		return -EINVAL;
> +	}
> +
>  	if (strlen(opt) >= MAX_CONFIG_LEN) {
>  		printk(KERN_ERR "kgdbts: config string too long\n");
>  		return -ENOSPC;
> -- 
> 2.7.4
> 

[PATCH] kgdbts: Passing ekgdbts to command line causes panic

[Defang Bo]

Similar to commit<1bd54d851f50>("kgdboc: Passing ekgdboc to command line causes panic"),
kgdbts_option_setup does not check input argument before passing it to strlen.
The argument would be a NULL pointer.

Signed-off-by: Defang Bo <bodefang@126.com>
---
 drivers/misc/kgdbts.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/misc/kgdbts.c b/drivers/misc/kgdbts.c
index 945701b..b077547 100644
--- a/drivers/misc/kgdbts.c
+++ b/drivers/misc/kgdbts.c
@@ -1057,6 +1057,11 @@ static void kgdbts_run_tests(void)
 
 static int kgdbts_option_setup(char *opt)
 {
+	if (!opt) {
+		pr_err("kgdbts: config string not provided\n");
+		return -EINVAL;
+	}
+
 	if (strlen(opt) >= MAX_CONFIG_LEN) {
 		printk(KERN_ERR "kgdbts: config string too long\n");
 		return -ENOSPC;
-- 
2.7.4

[PATCH] kgdbts: Passing ekgdbts to command line causes panic

[Defang Bo]

Similar to commit<1bd54d851f50>(("kgdboc: Passing ekgdboc to command line causes panic"),
kgdbts_option_setup does not check input argument before passing it to strlen.
The argument would be a NULL pointer.

Signed-off-by: Defang Bo <bodefang@126.com>
---
 drivers/misc/kgdbts.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/misc/kgdbts.c b/drivers/misc/kgdbts.c
index 945701b..b077547 100644
--- a/drivers/misc/kgdbts.c
+++ b/drivers/misc/kgdbts.c
@@ -1057,6 +1057,11 @@ static void kgdbts_run_tests(void)
 
 static int kgdbts_option_setup(char *opt)
 {
+	if (!opt) {
+		pr_err("kgdbts: config string not provided\n");
+		return -EINVAL;
+	}
+
 	if (strlen(opt) >= MAX_CONFIG_LEN) {
 		printk(KERN_ERR "kgdbts: config string too long\n");
 		return -ENOSPC;
-- 
2.7.4