linux-kernel - Re: [PATCH v3] n_tty: Fix buffer overruns with larger-than-4k pastes

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-next>] [day] [month] [year] [list]

Message-ID: <52A5F96A.6030900@hurleysoftware.com>
Date:	Mon, 09 Dec 2013 12:10:02 -0500
From:	Peter Hurley <peter@...leysoftware.com>
To:	Stas Sergeev <stsp@...t.ru>
CC:	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Margarita Manterola <margamanterola@...il.com>,
	Maximiliano Curia <maxy@...servers.com.ar>,
	Stas Sergeev <stsp@...rs.sourceforge.net>,
	Pavel Machek <pavel@....cz>,
	Arkadiusz Miskiewicz <a.miskiewicz@...il.com>,
	One Thousand Gnomes <gnomes@...rguk.ukuu.org.uk>,
	linux-kernel@...r.kernel.org, Caylan Van Larson <i@...lan.net>
Subject: Re: [PATCH v3] n_tty: Fix buffer overruns with larger-than-4k pastes

On 12/09/2013 11:26 AM, Stas Sergeev wrote:
> 09.12.2013 18:50, Peter Hurley пишет:
>>   	if (found && read_buf(ldata, eol) == __DISABLED_CHAR) {
>>   		n--;
>>   		eof_push = !n && ldata->read_tail != ldata->line_start;
>> +		ldata->push = 0;
>>   	}
> Will this work if the last (and only) char written in raw
> mode appear to be \0 (__DISABLED_CHAR)?

That would have triggered an EOF in older kernels so not a
regression.

>
>> -		ldata->line_start = ldata->read_tail;
>> +		if (!ldata->push)
>> +			ldata->line_start = ldata->read_tail;
>> +		else
>> +			ldata->push = 0;
> Will this work if more that one "fake" EOF is accumulated
> in bitmap because of multiple icanon switches?

Not possible. The read_flags and push indicator are reset
with icanon -> !icanon switches.

> Also, I am a bit surprised with the presence of the code
> like this:
> ---
>
> |	if  (n>  4096)
> 		n+=  4096;
> |
>
> ---
> Am I the only one thinking it is unclear what it does?
> Doesn't it deserve the comment at least?
>
> Or this:
> ---
>
> |eof_push= !n&&  ldata->read_tail!=  ldata->line_start;|
>
> ---
> If eof_push means that the EOL mark was found not at the
> line start, then it is completely confusing why !n is here
> (one have to read a lot of context to find out why).|||
> When I created the patch, the code was much more easy
> to follow than now.

The previous kernels did byte-by-byte copy with lock/unlock
for every byte, into the input buffer and back out of the input
buffer. Simple, but inefficient.

This version has roughly 4n-2 fewer bus locks. The necessary
unsigned arithmetic and additional condition checks make the
code somewhat more complex.

But please feel free to submit patches that fix or clarify
anything you find.

Regards,
Peter Hurley
--
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/