[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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
