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@xxxxxxxxxxxxx> wrote:
>>
>>> On 2018å09æ19æ 10:43, Steven Rostedt wrote:
>>>> On Wed, 19 Sep 2018 11:39:32 +0900
>>>> Sergey Senozhatsky <sergey.senozhatsky.work@xxxxxxxxx> 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@xxxxxxxxxxxxx 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
>