linux-kernel - Re: [PATCH 1/2] ras: fix an off-by-one error in __find

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]

Message-ID: <20190416221852.GA10781@agluck-desk>
Date:   Tue, 16 Apr 2019 15:18:52 -0700
From:   "Luck, Tony" <tony.luck@...el.com>
To:     Borislav Petkov <bp@...en8.de>
Cc:     Cong Wang <xiyou.wangcong@...il.com>, linux-kernel@...r.kernel.org,
        linux-edac@...r.kernel.org, Thomas Gleixner <tglx@...utronix.de>
Subject: Re: [PATCH 1/2] ras: fix an off-by-one error in __find_elem()

On Tue, Apr 16, 2019 at 11:07:26AM +0200, Borislav Petkov wrote:
> On Mon, Apr 15, 2019 at 06:20:00PM -0700, Cong Wang wrote:
> > ce_arr.array[] is always within the range [0, ce_arr.n-1].
> > However, the binary search code in __find_elem() uses ce_arr.n
> > as the maximum index, which could lead to an off-by-one
> > out-of-bound access when the element after the last is exactly
> > the one just got deleted, that is, 'min' returned to caller as
> > 'ce_arr.n'.
> 
> Sorry, I don't follow.
> 
> There's a debugfs interface in /sys/kernel/debug/ras/cec/ with which you
> can input random PFNs and test the thing.
> 
> Show me pls how this can happen with an example.

The array of previously seen pfn values is one page.

The problem case occurs when we've seen enough distinct
errors that we have filled every entry, then we try to
look up a pfn that is larger that any seen before.

The loop:

	while (min < max) {
		...
	}

will terminate with "min" set to MAX_ELEMS. Then we
execute:

	this_pfn = PFN(ca->array[min]);

which references beyond the end of the space allocated
for ca->array.

Probably won't crash, but we will read a garbage value
from whatever memory is allocated next.

Chances are high that the test:

	if (this_pfn == pfn)

won't find that the garbage value matches the pfn that
we were looking for ... so we will likley be lucky and
not do anything too dumb. But we shouldn't just cross
our fingers and hope.

Fix looks mostly OK, but we should probably move the

	if (to)
		*to = min;

inside the new

	if (min < ca->n) {
		...
	}

clause.

-Tony