[PATCH v2 1/2] printk: Fix panic caused by passing log_buf_len to command line

[PATCH v2 1/2] printk: Fix panic caused by passing log_buf_len to command line

[zhe.he]

From: He Zhe <zhe.he@windriver.com>

log_buf_len_setup does not check input argument before passing it to
simple_strtoull. The argument would be a NULL pointer if "log_buf_len",
without its value, is set in command line and thus causes the following
panic.

PANIC: early exception 0xe3 IP 10:ffffffffaaeacd0d error 0 cr2 0x0
[    0.000000] CPU: 0 PID: 0 Comm: swapper Not tainted 4.19.0-rc4-yocto-standard+ #1
[    0.000000] RIP: 0010:_parse_integer_fixup_radix+0xd/0x70
...
[    0.000000] Call Trace:
[    0.000000]  simple_strtoull+0x29/0x70
[    0.000000]  memparse+0x26/0x90
[    0.000000]  log_buf_len_setup+0x17/0x22
[    0.000000]  do_early_param+0x57/0x8e
[    0.000000]  parse_args+0x208/0x320
[    0.000000]  ? rdinit_setup+0x30/0x30
[    0.000000]  parse_early_options+0x29/0x2d
[    0.000000]  ? rdinit_setup+0x30/0x30
[    0.000000]  parse_early_param+0x36/0x4d
[    0.000000]  setup_arch+0x336/0x99e
[    0.000000]  start_kernel+0x6f/0x4ee
[    0.000000]  x86_64_start_reservations+0x24/0x26
[    0.000000]  x86_64_start_kernel+0x6f/0x72
[    0.000000]  secondary_startup_64+0xa4/0xb0

This patch adds a check to prevent the panic.

Signed-off-by: He Zhe <zhe.he@windriver.com>
Cc: stable@vger.kernel.org
Cc: pmladek@suse.com
Cc: sergey.senozhatsky@gmail.com
Cc: rostedt@goodmis.org
---
v2:
Split out the addition of pr_fmt and the unsigned update

 kernel/printk/printk.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 9bf5404..34c0403 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -1048,7 +1048,14 @@ static void __init log_buf_len_update(unsigned size)
 /* save requested log_buf_len since it's too early to process it */
 static int __init log_buf_len_setup(char *str)
 {
-	unsigned size = memparse(str, &str);
+	unsigned size;
+
+	if (!str) {
+		pr_err("Config string not provided\n");
+		return -EINVAL;
+	}
+
+	size = memparse(str, &str);
 
 	log_buf_len_update(size);
 
-- 
2.7.4

[PATCH v2 2/2] printk: Add KBUILD_MODNAME and correct bare use of unsigned

[zhe.he]

From: He Zhe <zhe.he@windriver.com>

Add KBUILD_MODNAME to make prints more clear. And use 'unsigned int' intead
of 'unsigned' according to checkpatch.pl's suggestion.

Signed-off-by: He Zhe <zhe.he@windriver.com>
Cc: pmladek@suse.com
Cc: sergey.senozhatsky@gmail.com
Cc: rostedt@goodmis.org
---
v2:
Correct one more place

 kernel/printk/printk.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 34c0403..ece870f 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -16,6 +16,8 @@
  *	01Mar01 Andrew Morton
  */
 
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
 #include <linux/kernel.h>
 #include <linux/mm.h>
 #include <linux/tty.h>
@@ -1048,7 +1050,7 @@ static void __init log_buf_len_update(unsigned size)
 /* save requested log_buf_len since it's too early to process it */
 static int __init log_buf_len_setup(char *str)
 {
-	unsigned size;
+	unsigned int size;
 
 	if (!str) {
 		pr_err("Config string not provided\n");
@@ -2359,7 +2361,7 @@ void console_unlock(void)
 		raw_spin_lock(&logbuf_lock);
 		if (console_seq < log_first_seq) {
 			len = sprintf(text, "** %u printk messages dropped **\n",
-				      (unsigned)(log_first_seq - console_seq));
+				   (unsigned int)(log_first_seq - console_seq));
 
 			/* messages are gone, move to first one */
 			console_seq = log_first_seq;
-- 
2.7.4

Re: [PATCH v2 1/2] printk: Fix panic caused by passing log_buf_len to command line

[Sergey Senozhatsky]

On (09/19/18 01:17), zhe.he@windriver.com wrote:
> @@ -1048,7 +1048,14 @@ static void __init log_buf_len_update(unsigned size)
>  /* save requested log_buf_len since it's too early to process it */
>  static int __init log_buf_len_setup(char *str)
>  {
> -	unsigned size = memparse(str, &str);
> +	unsigned size;

	unsigned int size;

> +	if (!str) {
> +		pr_err("Config string not provided\n");

pr_debug() may be?

It's not clear from this error message which one of the "config strings"
was not provided. Besides, I think "config string" is misleading and in
fact it's a "boot command line parameter". But, dunno, I guess I'd just
drop that print out.

Otherwise,

Acked-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>

	-ss

Re: [PATCH v2 2/2] printk: Add KBUILD_MODNAME and correct bare use of unsigned

[Sergey Senozhatsky]

On (09/19/18 01:17), zhe.he@windriver.com wrote:
> Add KBUILD_MODNAME to make prints more clear.

No strong opinion. I'm OK with this change.

> And use 'unsigned int' intead of 'unsigned' according to
> checkpatch.pl's suggestion.

I don't think that "unsigned int" is the right thing to use there.

>  		if (console_seq < log_first_seq) {
>  			len = sprintf(text, "** %u printk messages dropped **\n",
> -				      (unsigned)(log_first_seq - console_seq));
> +				   (unsigned int)(log_first_seq - console_seq));

Both log_first_seq and console_seq are u64.

	log_first_seq - console_seq

thus, *in theory*, can be larger than "unsigned int". So I'd just avoid cast
and use an appropriate for u64 %llu sprintf() specifier. Something like below,
perhaps:

---

diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index f73ea9dd6f46..4b8c5832bf14 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -2408,8 +2408,9 @@ void console_unlock(void)
 		printk_safe_enter_irqsave(flags);
 		raw_spin_lock(&logbuf_lock);
 		if (console_seq < log_first_seq) {
-			len = sprintf(text, "** %u printk messages dropped **\n",
-				   (unsigned int)(log_first_seq - console_seq));
+			len = sprintf(text,
+				      "** %llu printk messages dropped **\n",
+				      log_first_seq - console_seq);
 
 			/* messages are gone, move to first one */
 			console_seq = log_first_seq;

---

Steven, Petr, any objections?

	-ss

Re: [PATCH v2 1/2] printk: Fix panic caused by passing log_buf_len to command line

[He Zhe]

On 2018年09月19日 09:50, Sergey Senozhatsky wrote:
> On (09/19/18 01:17), zhe.he@windriver.com wrote:
>> @@ -1048,7 +1048,14 @@ static void __init log_buf_len_update(unsigned size)
>>  /* save requested log_buf_len since it's too early to process it */
>>  static int __init log_buf_len_setup(char *str)
>>  {
>> -	unsigned size = memparse(str, &str);
>> +	unsigned size;
> 	unsigned int size;

This is in v1 but then Steven suggested that it should be split out
and only keep the pure fix part here.

>
>> +	if (!str) {
>> +		pr_err("Config string not provided\n");
> pr_debug() may be?
>
> It's not clear from this error message which one of the "config strings"
> was not provided. Besides, I think "config string" is misleading and in
> fact it's a "boot command line parameter". But, dunno, I guess I'd just
> drop that print out.

I'm OK with dropping it. Anyway it'd get "Malformed early
option 'log_buf_len'" from early_param later.

I'll send v3. Thank you.

Zhe

>
> Otherwise,
>
> Acked-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
>
> 	-ss
>

Re: [PATCH v2 1/2] printk: Fix panic caused by passing log_buf_len to command line

[Sergey Senozhatsky]

On (09/19/18 10:27), He Zhe wrote:
> On 2018年09月19日 09:50, Sergey Senozhatsky wrote:
> > On (09/19/18 01:17), zhe.he@windriver.com wrote:
> >> @@ -1048,7 +1048,14 @@ static void __init log_buf_len_update(unsigned size)
> >>  /* save requested log_buf_len since it's too early to process it */
> >>  static int __init log_buf_len_setup(char *str)
> >>  {
> >> -	unsigned size = memparse(str, &str);
> >> +	unsigned size;
> > 	unsigned int size;
> 
> This is in v1 but then Steven suggested that it should be split out
> and only keep the pure fix part here.

Ah, I see.

Hmm... memparse() returns u64 value. A user *probably* can ask the kernel
to allocate log_buf larger than 'unsigned int'.

So may be I'd do two fixes here:

 First  - switch to u64 size.
 Second - check for NULL str.


Steven, Petr, what do you think?

	-ss

Re: [PATCH v2 1/2] printk: Fix panic caused by passing log_buf_len to command line

[Steven Rostedt]

On Wed, 19 Sep 2018 11:39:32 +0900
Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> wrote:

> On (09/19/18 10:27), He Zhe wrote:
> > On 2018年09月19日 09:50, Sergey Senozhatsky wrote:  
> > > On (09/19/18 01:17), zhe.he@windriver.com wrote:  
> > >> @@ -1048,7 +1048,14 @@ static void __init log_buf_len_update(unsigned size)
> > >>  /* save requested log_buf_len since it's too early to process it */
> > >>  static int __init log_buf_len_setup(char *str)
> > >>  {
> > >> -	unsigned size = memparse(str, &str);
> > >> +	unsigned size;  
> > > 	unsigned int size;  
> > 
> > This is in v1 but then Steven suggested that it should be split out
> > and only keep the pure fix part here.  
> 
> Ah, I see.
> 
> Hmm... memparse() returns u64 value. A user *probably* can ask the kernel
> to allocate log_buf larger than 'unsigned int'.
> 
> So may be I'd do two fixes here:
> 
>  First  - switch to u64 size.
>  Second - check for NULL str.
> 
> 
> Steven, Petr, what do you think?
> 

I think I would switch it around. Check for NULL first, and then switch
to u64. It was always an int, do we need to backport converting it to
u64 to stable? The NULL check is a definite, the overflow of int
shouldn't crash anything.

-- Steve

Re: [PATCH v2 1/2] printk: Fix panic caused by passing log_buf_len to command line

[Sergey Senozhatsky]

On (09/18/18 22:43), Steven Rostedt wrote:
> >  First  - switch to u64 size.
> >  Second - check for NULL str.
> > 
> I think I would switch it around. Check for NULL first, and then switch
> to u64. It was always an int, do we need to backport converting it to
> u64 to stable? The NULL check is a definite, the overflow of int
> shouldn't crash anything.

Agreed. This order makes much more sense. Do you mind, tho, to have
"unsigned int size" in the first patch along with NULL str check?
Just to silent the checkpatch.

	-ss

Re: [PATCH v2 1/2] printk: Fix panic caused by passing log_buf_len to command line

[Steven Rostedt]

On Wed, 19 Sep 2018 11:47:54 +0900
Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> wrote:

> On (09/18/18 22:43), Steven Rostedt wrote:
> > >  First  - switch to u64 size.
> > >  Second - check for NULL str.
> > >   
> > I think I would switch it around. Check for NULL first, and then switch
> > to u64. It was always an int, do we need to backport converting it to
> > u64 to stable? The NULL check is a definite, the overflow of int
> > shouldn't crash anything.  
> 
> Agreed. This order makes much more sense. Do you mind, tho, to have
> "unsigned int size" in the first patch along with NULL str check?
> Just to silent the checkpatch.
> 

I guess that doesn't hurt. I'd personally would keep it separate (just
fix what's broken), but it's such a slight change, I don't have any
strong feelings about it.

Thanks,


-- Steve

Re: [PATCH v2 1/2] printk: Fix panic caused by passing log_buf_len to command line

[Sergey Senozhatsky]

On (09/19/18 10:27), He Zhe wrote:
> On 2018年09月19日 09:50, Sergey Senozhatsky wrote:
> > On (09/19/18 01:17), zhe.he@windriver.com wrote:
> >> @@ -1048,7 +1048,14 @@ static void __init log_buf_len_update(unsigned size)
> >>  /* save requested log_buf_len since it's too early to process it */
> >>  static int __init log_buf_len_setup(char *str)
> >>  {
> >> -	unsigned size = memparse(str, &str);
> >> +	unsigned size;
> > 	unsigned int size;
> 
> This is in v1 but then Steven suggested that it should be split out
> and only keep the pure fix part here.

JFI

Seems there are more patches like this. I found this one in MM list:
lkml.kernel.org/r/1537284788-428784-1-git-send-email-zhe.he@windriver.com

Andrew's response:
lkml.kernel.org/r/20180918141917.2cb16b01c122dbe1ead2f657@linux-foundation.org

	-ss

Re: [PATCH v2 1/2] printk: Fix panic caused by passing log_buf_len to command line

[He Zhe]

On 2018年09月19日 14:44, Sergey Senozhatsky wrote:
> On (09/19/18 10:27), He Zhe wrote:
>> On 2018年09月19日 09:50, Sergey Senozhatsky wrote:
>>> On (09/19/18 01:17), zhe.he@windriver.com wrote:
>>>> @@ -1048,7 +1048,14 @@ static void __init log_buf_len_update(unsigned size)
>>>>  /* save requested log_buf_len since it's too early to process it */
>>>>  static int __init log_buf_len_setup(char *str)
>>>>  {
>>>> -	unsigned size = memparse(str, &str);
>>>> +	unsigned size;
>>> 	unsigned int size;
>> This is in v1 but then Steven suggested that it should be split out
>> and only keep the pure fix part here.
> JFI
>
> Seems there are more patches like this. I found this one in MM list:
> lkml.kernel.org/r/1537284788-428784-1-git-send-email-zhe.he@windriver.com
>
> Andrew's response:
> lkml.kernel.org/r/20180918141917.2cb16b01c122dbe1ead2f657@linux-foundation.org

I just replied. Let's see what Andrew would say.
https://lore.kernel.org/lkml/1c32c1d2-a54a-30f7-1afb-ad6d3282f54a@windriver.com/

Thanks,
Zhe

>
> 	-ss
>

Re: [PATCH v2 2/2] printk: Add KBUILD_MODNAME and correct bare use of unsigned

[Petr Mladek]

On Wed 2018-09-19 11:06:21, Sergey Senozhatsky wrote:
> On (09/19/18 01:17), zhe.he@windriver.com wrote:
> > Add KBUILD_MODNAME to make prints more clear.
> 
> No strong opinion. I'm OK with this change.
> 
> > And use 'unsigned int' intead of 'unsigned' according to
> > checkpatch.pl's suggestion.
> 
> I don't think that "unsigned int" is the right thing to use there.
> 
> >  		if (console_seq < log_first_seq) {
> >  			len = sprintf(text, "** %u printk messages dropped **\n",
> > -				      (unsigned)(log_first_seq - console_seq));
> > +				   (unsigned int)(log_first_seq - console_seq));
> 
> Both log_first_seq and console_seq are u64.
> 
> 	log_first_seq - console_seq
> 
> thus, *in theory*, can be larger than "unsigned int". So I'd just avoid cast
> and use an appropriate for u64 %llu sprintf() specifier. Something like below,
> perhaps:
> 
> ---
> 
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index f73ea9dd6f46..4b8c5832bf14 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -2408,8 +2408,9 @@ void console_unlock(void)
>  		printk_safe_enter_irqsave(flags);
>  		raw_spin_lock(&logbuf_lock);
>  		if (console_seq < log_first_seq) {
> -			len = sprintf(text, "** %u printk messages dropped **\n",
> -				   (unsigned int)(log_first_seq - console_seq));
> +			len = sprintf(text,
> +				      "** %llu printk messages dropped **\n",
> +				      log_first_seq - console_seq);

This looks like a better solution.

Also please, define pr_fmt and fix the casting in separate patches.

Best Regards,
Petr

Re: [PATCH v2 1/2] printk: Fix panic caused by passing log_buf_len to command line

[He Zhe]

On 2018年09月19日 10:43, Steven Rostedt wrote:
> On Wed, 19 Sep 2018 11:39:32 +0900
> Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> wrote:
>
>> On (09/19/18 10:27), He Zhe wrote:
>>> On 2018年09月19日 09:50, Sergey Senozhatsky wrote:  
>>>> On (09/19/18 01:17), zhe.he@windriver.com wrote:  
>>>>> @@ -1048,7 +1048,14 @@ static void __init log_buf_len_update(unsigned size)
>>>>>  /* save requested log_buf_len since it's too early to process it */
>>>>>  static int __init log_buf_len_setup(char *str)
>>>>>  {
>>>>> -	unsigned size = memparse(str, &str);
>>>>> +	unsigned size;  
>>>> 	unsigned int size;  
>>> This is in v1 but then Steven suggested that it should be split out
>>> and only keep the pure fix part here.  
>> Ah, I see.
>>
>> Hmm... memparse() returns u64 value. A user *probably* can ask the kernel
>> to allocate log_buf larger than 'unsigned int'.
>>
>> So may be I'd do two fixes here:
>>
>>  First  - switch to u64 size.
>>  Second - check for NULL str.
>>
>>
>> Steven, Petr, what do you think?
>>
> I think I would switch it around. Check for NULL first, and then switch
> to u64. It was always an int, do we need to backport converting it to
> u64 to stable? The NULL check is a definite, the overflow of int
> shouldn't crash anything.

To switch to u64, several variables need to be adjusted to new type to aligned
with new_log_buf_len. And currently new_log_buf_len is passed to
memblock_virt_alloc(phys_addr_t, phys_addr_t). So we can't simply define
new_log_buf_len as u64. We need to define it as phys_addr_t tomake it work
well for both 32bit and 64bit arches, since a 32-bit architecture can set
ARCH_PHYS_ADDR_T_64BIT if it needs a 64-bit phys_addr_t.

What do you think?

#ifdef CONFIG_PHYS_ADDR_T_64BIT
typedef u64 phys_addr_t;
#else
typedef u32 phys_addr_t;
#endif

Thanks,
Zhe


> -- Steve
>

Re: [PATCH v2 1/2] printk: Fix panic caused by passing log_buf_len to command line

[Steven Rostedt]

On Fri, 21 Sep 2018 00:16:50 +0800
He Zhe <zhe.he@windriver.com> wrote:

> On 2018年09月19日 10:43, Steven Rostedt wrote:
> > On Wed, 19 Sep 2018 11:39:32 +0900
> > Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> wrote:
> >  
> >> On (09/19/18 10:27), He Zhe wrote:  
> >>> On 2018年09月19日 09:50, Sergey Senozhatsky wrote:    
> >>>> On (09/19/18 01:17), zhe.he@windriver.com wrote:    
> >>>>> @@ -1048,7 +1048,14 @@ static void __init log_buf_len_update(unsigned size)
> >>>>>  /* save requested log_buf_len since it's too early to process it */
> >>>>>  static int __init log_buf_len_setup(char *str)
> >>>>>  {
> >>>>> -	unsigned size = memparse(str, &str);
> >>>>> +	unsigned size;    
> >>>> 	unsigned int size;    
> >>> This is in v1 but then Steven suggested that it should be split out
> >>> and only keep the pure fix part here.    
> >> Ah, I see.
> >>
> >> Hmm... memparse() returns u64 value. A user *probably* can ask the kernel
> >> to allocate log_buf larger than 'unsigned int'.
> >>
> >> So may be I'd do two fixes here:
> >>
> >>  First  - switch to u64 size.
> >>  Second - check for NULL str.
> >>
> >>
> >> Steven, Petr, what do you think?
> >>  
> > I think I would switch it around. Check for NULL first, and then switch
> > to u64. It was always an int, do we need to backport converting it to
> > u64 to stable? The NULL check is a definite, the overflow of int
> > shouldn't crash anything.  
> 

Hi Zhe,

> To switch to u64, several variables need to be adjusted to new type to aligned
> with new_log_buf_len. And currently new_log_buf_len is passed to
> memblock_virt_alloc(phys_addr_t, phys_addr_t). So we can't simply define
> new_log_buf_len as u64. We need to define it as phys_addr_t tomake it work
> well for both 32bit and 64bit arches, since a 32-bit architecture can set
> ARCH_PHYS_ADDR_T_64BIT if it needs a 64-bit phys_addr_t.

The above explanation verifies more that the NULL pointer check needs
to be first, and that the change in size should not be backported to
stable because it has a high risk to doing the change as compared to it
being a problem for older kernels.

> 
> What do you think?

Sounds good to me.

What do others think?

-- Steve

Re: [PATCH v2 1/2] printk: Fix panic caused by passing log_buf_len to command line

[Petr Mladek]

On Thu 2018-09-20 12:30:56, Steven Rostedt wrote:
> On Fri, 21 Sep 2018 00:16:50 +0800
> He Zhe <zhe.he@windriver.com> wrote:
> 
> > On 2018年09月19日 10:43, Steven Rostedt wrote:
> > > On Wed, 19 Sep 2018 11:39:32 +0900
> > > Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> wrote:
> > >  
> > >> On (09/19/18 10:27), He Zhe wrote:  
> > >>> On 2018年09月19日 09:50, Sergey Senozhatsky wrote:    
> > >>>> On (09/19/18 01:17), zhe.he@windriver.com wrote:    
> > >>>>> @@ -1048,7 +1048,14 @@ static void __init log_buf_len_update(unsigned size)
> > >>>>>  /* save requested log_buf_len since it's too early to process it */
> > >>>>>  static int __init log_buf_len_setup(char *str)
> > >>>>>  {
> > >>>>> -	unsigned size = memparse(str, &str);
> > >>>>> +	unsigned size;    
> > >>>> 	unsigned int size;    
> > >>> This is in v1 but then Steven suggested that it should be split out
> > >>> and only keep the pure fix part here.    
> > >> Ah, I see.
> > >>
> > >> Hmm... memparse() returns u64 value. A user *probably* can ask the kernel
> > >> to allocate log_buf larger than 'unsigned int'.
> > >>
> > >> So may be I'd do two fixes here:
> > >>
> > >>  First  - switch to u64 size.
> > >>  Second - check for NULL str.
> > >>
> > >>
> > >> Steven, Petr, what do you think?
> > >>  
> > > I think I would switch it around. Check for NULL first, and then switch
> > > to u64. It was always an int, do we need to backport converting it to
> > > u64 to stable? The NULL check is a definite, the overflow of int
> > > shouldn't crash anything.  
> > 
> 
> Hi Zhe,
> 
> > To switch to u64, several variables need to be adjusted to new type to aligned
> > with new_log_buf_len. And currently new_log_buf_len is passed to
> > memblock_virt_alloc(phys_addr_t, phys_addr_t). So we can't simply define
> > new_log_buf_len as u64. We need to define it as phys_addr_t tomake it work
> > well for both 32bit and 64bit arches, since a 32-bit architecture can set
> > ARCH_PHYS_ADDR_T_64BIT if it needs a 64-bit phys_addr_t.
> 
> The above explanation verifies more that the NULL pointer check needs
> to be first, and that the change in size should not be backported to
> stable because it has a high risk to doing the change as compared to it
> being a problem for older kernels.

I agree that the NULL check should go first.

I would personally keep the size as unsigned int. IMHO, a support
for a log buffer bigger than 4GB is not worth the complexity.

Best Regards,
Petr

Re: [PATCH v2 1/2] printk: Fix panic caused by passing log_buf_len to command line

[He Zhe]

On 2018年09月21日 15:37, Petr Mladek wrote:
> On Thu 2018-09-20 12:30:56, Steven Rostedt wrote:
>> On Fri, 21 Sep 2018 00:16:50 +0800
>> He Zhe <zhe.he@windriver.com> wrote:
>>
>>> On 2018年09月19日 10:43, Steven Rostedt wrote:
>>>> On Wed, 19 Sep 2018 11:39:32 +0900
>>>> Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> wrote:
>>>>  
>>>>> On (09/19/18 10:27), He Zhe wrote:  
>>>>>> On 2018年09月19日 09:50, Sergey Senozhatsky wrote:    
>>>>>>> On (09/19/18 01:17), zhe.he@windriver.com wrote:    
>>>>>>>> @@ -1048,7 +1048,14 @@ static void __init log_buf_len_update(unsigned size)
>>>>>>>>  /* save requested log_buf_len since it's too early to process it */
>>>>>>>>  static int __init log_buf_len_setup(char *str)
>>>>>>>>  {
>>>>>>>> -	unsigned size = memparse(str, &str);
>>>>>>>> +	unsigned size;    
>>>>>>> 	unsigned int size;    
>>>>>> This is in v1 but then Steven suggested that it should be split out
>>>>>> and only keep the pure fix part here.    
>>>>> Ah, I see.
>>>>>
>>>>> Hmm... memparse() returns u64 value. A user *probably* can ask the kernel
>>>>> to allocate log_buf larger than 'unsigned int'.
>>>>>
>>>>> So may be I'd do two fixes here:
>>>>>
>>>>>  First  - switch to u64 size.
>>>>>  Second - check for NULL str.
>>>>>
>>>>>
>>>>> Steven, Petr, what do you think?
>>>>>  
>>>> I think I would switch it around. Check for NULL first, and then switch
>>>> to u64. It was always an int, do we need to backport converting it to
>>>> u64 to stable? The NULL check is a definite, the overflow of int
>>>> shouldn't crash anything.  
>> Hi Zhe,
>>
>>> To switch to u64, several variables need to be adjusted to new type to aligned
>>> with new_log_buf_len. And currently new_log_buf_len is passed to
>>> memblock_virt_alloc(phys_addr_t, phys_addr_t). So we can't simply define
>>> new_log_buf_len as u64. We need to define it as phys_addr_t tomake it work
>>> well for both 32bit and 64bit arches, since a 32-bit architecture can set
>>> ARCH_PHYS_ADDR_T_64BIT if it needs a 64-bit phys_addr_t.
>> The above explanation verifies more that the NULL pointer check needs
>> to be first, and that the change in size should not be backported to
>> stable because it has a high risk to doing the change as compared to it
>> being a problem for older kernels.
> I agree that the NULL check should go first.
>
> I would personally keep the size as unsigned int. IMHO, a support
> for a log buffer bigger than 4GB is not worth the complexity.

Thank you, Petr, Steven and Sergey. I'll send v3 soon for the NULL check
and cast correction.



For 64 bit log buffer length, the variable changes should be OK. The main
obstacle might be that we need to modify the syscall below to make the
64 bit length returned back to user space. "error" is not long enough.

int do_syslog(int type, char __user *buf, int len, int source)
...
        case SYSLOG_ACTION_SIZE_BUFFER:                                         
                error = log_buf_len;                                            
                break;
...
    return error;
...

SYSCALL_DEFINE3(syslog, int, type, char __user *, buf, int, len)                
{                                                                                                                                                                                       
        return do_syslog(type, buf, len, SYSLOG_FROM_READER);                       
}



Besides, in terms of variable type alignment, there has already been a
similar problematic case in do_syslog as below. i.e. int = u64 - u64. It seems
this needs to be fixed.

int do_syslog(int type, char __user *buf, int len, int source)
...
    case SYSLOG_ACTION_SIZE_UNREAD:
        if (source == SYSLOG_FROM_PROC) {
            error = log_next_seq - syslog_seq;
...



I'd like to fix the above issues. But can I have your opinions about how to
handle the syscall?


Thanks,
Zhe

>
> Best Regards,
> Petr
>

Re: [PATCH v2 1/2] printk: Fix panic caused by passing log_buf_len to command line

[Sergey Senozhatsky]

On (09/21/18 09:37), Petr Mladek wrote:
> 
> I would personally keep the size as unsigned int. IMHO, a support
> for a log buffer bigger than 4GB is not worth the complexity.
> 

ftrace dumps are bothering me.

Steven Rostedt wrote [0]:
>
> Especially when I have a machine with 240 CPUs. But it also has a ton of
> RAM, I could easily do log_buf_len=32G
>

The systems are getting bigger, so log_buf_len=UINT_MAX+ might become
a norm at some point.

[0] https://lore.kernel.org/lkml/20170428101659.7cd879e7@gandalf.local.home/

	-ss

Re: [PATCH v2 1/2] printk: Fix panic caused by passing log_buf_len to command line

[Sergey Senozhatsky]

On (09/22/18 23:36), He Zhe wrote:
> I'd like to fix the above issues. But can I have your opinions about how to
> handle the syscall?

Hmm. This part is complex, I need to look at it more.
A signed int is a good buffer size limit for 32-bit user space.
But this also means that syslog might experience some problems
doing SYSLOG_ACTION_READ_ALL on a 32-bit system with a 4G log_buf.
Not to mention SYSLOG_ACTION_READ_ALL on a 64-bit system with a
log_buf=32G. So *maybe* things are alredy a bit broken in this
department. Dunno, need to think more.

	-ss

Re: [PATCH v2 1/2] printk: Fix panic caused by passing log_buf_len to command line

[Petr Mladek]

On Tue 2018-09-25 21:01:35, Sergey Senozhatsky wrote:
> On (09/21/18 09:37), Petr Mladek wrote:
> > 
> > I would personally keep the size as unsigned int. IMHO, a support
> > for a log buffer bigger than 4GB is not worth the complexity.
> > 
> 
> ftrace dumps are bothering me.
> 
> Steven Rostedt wrote [0]:
> >
> > Especially when I have a machine with 240 CPUs. But it also has a ton of
> > RAM, I could easily do log_buf_len=32G
> >
> 
> The systems are getting bigger, so log_buf_len=UINT_MAX+ might become
> a norm at some point.

Thanks for pointing this out. Well, it seems that the change would
require a new syscall to pass the buffer size as long. We need to
be sure that people would use this in the real life.

This thread suggested this change to avoid a checkpatch warning.
The 32GB was mentioned as an example one year ego. This is not enough
for a new syscall from my point of view.

Best Regards,
Petr

Re: [PATCH v2 1/2] printk: Fix panic caused by passing log_buf_len to command line

[Sergey Senozhatsky]

On (09/25/18 14:23), Petr Mladek wrote:
> 
> Thanks for pointing this out. Well, it seems that the change would
> require a new syscall to pass the buffer size as long. We need to
> be sure that people would use this in the real life.

Agreed.

> This thread suggested this change to avoid a checkpatch warning.

Not exactly. I suggested u64 change not because of a checkpatch
warning. But because of u64 memparse() return and because of potential
log_buf_len=4G+

	-ss

Re: [PATCH v2 1/2] printk: Fix panic caused by passing log_buf_len to command line

[Sergey Senozhatsky]

On (09/25/18 14:23), Petr Mladek wrote:
> The 32GB was mentioned as an example one year ego. This is not enough
> for a new syscall from my point of view.

I agree. I didn't think of syslog(); was merely thinking about logbuf
and flushing it to the consoles. syslog() stuff is a bit complex. We
sort of don't expect user space to allocate 64G to read all log_buf
messages, do we.

I'm wondering if we can do something like this

---

diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index cf275f4d7912..1b48b61da8fe 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -1110,9 +1110,15 @@ static void __init log_buf_len_update(unsigned size)
 /* save requested log_buf_len since it's too early to process it */
 static int __init log_buf_len_setup(char *str)
 {
-	unsigned size = memparse(str, &str);
+	u64 size = memparse(str, &str);
 
-	log_buf_len_update(size);
+	if (size > UINT_MAX) {
+		size = UINT_MAX;
+		pr_err("log_buf over 4G is not supported. "
+			"Please contact printk maintainers.\n");
+	}
+
+	log_buf_len_update((unsigned int)size);
 
 	return 0;
 }

---

So we could know that "the day has come".

	-ss

Re: [PATCH v2 1/2] printk: Fix panic caused by passing log_buf_len to command line

[He Zhe]

On 2018年09月25日 21:31, Sergey Senozhatsky wrote:
> On (09/25/18 14:23), Petr Mladek wrote:
>> The 32GB was mentioned as an example one year ego. This is not enough
>> for a new syscall from my point of view.
> I agree. I didn't think of syslog(); was merely thinking about logbuf
> and flushing it to the consoles. syslog() stuff is a bit complex. We
> sort of don't expect user space to allocate 64G to read all log_buf
> messages, do we.
>
> I'm wondering if we can do something like this
>
> ---
>
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index cf275f4d7912..1b48b61da8fe 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -1110,9 +1110,15 @@ static void __init log_buf_len_update(unsigned size)
>  /* save requested log_buf_len since it's too early to process it */
>  static int __init log_buf_len_setup(char *str)
>  {
> -	unsigned size = memparse(str, &str);
> +	u64 size = memparse(str, &str);
>  
> -	log_buf_len_update(size);
> +	if (size > UINT_MAX) {
> +		size = UINT_MAX;
> +		pr_err("log_buf over 4G is not supported. "
> +			"Please contact printk maintainers.\n");
> +	}
> +
> +	log_buf_len_update((unsigned int)size);
>  
>  	return 0;
>  }
>
> ---
>
> So we could know that "the day has come".

I agree on this idea, a good way to collect the potential requirement.
I can send v4 with it if no objection from other people.

Thanks,
Zhe

>
> 	-ss
>

Re: [PATCH v2 1/2] printk: Fix panic caused by passing log_buf_len to command line

[Petr Mladek]

On Tue 2018-09-25 22:31:43, Sergey Senozhatsky wrote:
> On (09/25/18 14:23), Petr Mladek wrote:
> > The 32GB was mentioned as an example one year ego. This is not enough
> > for a new syscall from my point of view.
> 
> I agree. I didn't think of syslog(); was merely thinking about logbuf
> and flushing it to the consoles. syslog() stuff is a bit complex. We
> sort of don't expect user space to allocate 64G to read all log_buf
> messages, do we.
> 
> I'm wondering if we can do something like this
> 
> ---
> 
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index cf275f4d7912..1b48b61da8fe 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -1110,9 +1110,15 @@ static void __init log_buf_len_update(unsigned size)
>  /* save requested log_buf_len since it's too early to process it */
>  static int __init log_buf_len_setup(char *str)
>  {
> -	unsigned size = memparse(str, &str);
> +	u64 size = memparse(str, &str);
>  
> -	log_buf_len_update(size);
> +	if (size > UINT_MAX) {
> +		size = UINT_MAX;
> +		pr_err("log_buf over 4G is not supported. "
> +			"Please contact printk maintainers.\n");
> +	}
> +
> +	log_buf_len_update((unsigned int)size);
>  
>  	return 0;
>  }
> 
> ---
> 
> So we could know that "the day has come".

Sounds good to me. Just two nits.

First, I would move the check into log_buf_len_update() so that
we catch the overflow also in other situations.

Second, please, keep only the first line. It is enough to describe
the problem. Upstream kernel maintainers are not responsible
for implementing all missing features.

Best Regards,
Petr

Re: [PATCH v2 1/2] printk: Fix panic caused by passing log_buf_len to command line

[Sergey Senozhatsky]

On (09/26/18 13:05), Petr Mladek wrote:
> First, I would move the check into log_buf_len_update() so that
> we catch the overflow also in other situations.

OK.

> Second, please, keep only the first line.

OK.

> It is enough to describe the problem.

OK.
He Zhe, will you pick it up?

	-ss

Re: [PATCH v2 1/2] printk: Fix panic caused by passing log_buf_len to command line

[He Zhe]

On 2018年09月28日 15:35, Sergey Senozhatsky wrote:
> On (09/26/18 13:05), Petr Mladek wrote:
>> First, I would move the check into log_buf_len_update() so that
>> we catch the overflow also in other situations.
> OK.
>
>> Second, please, keep only the first line.
> OK.
>
>> It is enough to describe the problem.
> OK.
> He Zhe, will you pick it up?

Yes, I agree, I'll send v3 ASAP.

Thanks,
Zhe

>
> 	-ss
>