linux-kernel - Re: [ 1/1] printk: fix buffer overflow when calling log_prefix function from call_console

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 20 Feb 2013 14:43:09 +0100
From:	Alexandre SIMON <Alexandre.Simon@...v-lorraine.fr>
To:	Satoru Takeuchi <satoru.takeuchi@...il.com>
CC:	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	linux-kernel@...r.kernel.org, stable@...r.kernel.org,
	Alexandre SIMON <Alexandre.Simon@...v-lorraine.fr>
Subject: Re: [ 1/1] printk: fix buffer overflow when calling log_prefix function
 from call_console_drivers

[Satoru Takeuchi] wrote the following on [20/02/2013 14:02]:
[...]
> 
> I reviewed this patch and it seems to be good for me. Since I'm not good at
> printk code, I want to confirm whether what I think is correct or not.
> Is the following my understanding correct?

  No problem, it's nice to talk about this patch !


>> -			cur_index += log_prefix(&LOG_BUF(cur_index), &msg_level, NULL);
> 
> Here is one example of the problematic case.
> 
>   +---- start of log_buf
>   |
>   |                           +--- end of log_buf
>   |                           |
>   v                           v
>   <-------- log_buf ----------><------- * outside of log_buf. Don't access here !!! * --- ~~~
>                               ^
>                               |
>                               cur_index
> 
> In this case, only LOG_BUF(cur_index) is safe to access and
> 
>  - "LOG_BUF(cur_index) + 1" as p[1],
>  - "LOG_BUF(cur_index) + 2" as p[2], and
>  - "LOG_BUF(cur_index) + 1 or more" as simple_strtoul(&p[1], &endp, 10)
> 
> in log_prefix() are not to do so. Hence touching them would cause the system hang as you
> said as follows.

  Yes, your analyze is totally right. Your figure is clear and shows exactly when the problem occurs in the "borderline case".
  The initial code does not check that the "cur_index" can be at the end of "log_buf" whereas "log_prefix" function may access to one, two or more indexes after.


[...]
>> +			/*
>> +			 * prepare buf_prefix, as a contiguous array,
>> +			 * to be processed by log_prefix function
>> +			 */
>> +			char buf_prefix[SYSLOG_PRI_MAX_LENGTH+1];
>> +			unsigned i;
>> +			for (i = 0; i < ((end - cur_index)) && (i < SYSLOG_PRI_MAX_LENGTH); i++) {
> 
> The condition, "i < ((end - cur_index)) && (i < SYSLOG_PRI_MAX_LENGTH)", is to prevent
> access over
> 
>  - the region to write out, and

  Yes, because buf_prefix has a fixed length.


>  - the max length of log_prefix.

  In another term : to do not exceed the end boundary of "log_buf".


> In addition, "min(end - cur_index, SYSLOG_PRI_MAX_LENGTH)" has the same meaning here.

  Yes, that may also work.


>> +				buf_prefix[i] = LOG_BUF(cur_index + i);
> 
> You ensure that all characters (not only first character) in the candidate of log_prefix
> are inside of log_buf here by copying each character of them one by one.

  Yes, I also take care of the index that must be masked to wrap around the log_buf : we restart at index=0 when cur_index > length(log_buf).
  At this point, we are sure that the buffer passed to log_prefix is consistent and all the access (through p[1], p[2], simple_strtoul(...)) may not cause a buffer overflow.


> Thanks,
> Satoru

  Bye, Alex.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/