linux-kernel - Re: [PATCH] edac: fix buffer overrun if no suitable bandwidth found

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: <20121023213845.GA21115@aftab.osrc.amd.com>
Date:	Tue, 23 Oct 2012 23:38:45 +0200
From:	Borislav Petkov <bp@...64.org>
To:	Andrew Morton <akpm@...ux-foundation.org>
Cc:	Denis Kirjanov <kirjanov@...il.com>, linux-edac@...r.kernel.org,
	linux-kernel@...r.kernel.org, stable@...r.kernel.org
Subject: Re: [PATCH] edac: fix buffer overrun if no suitable bandwidth found

On Tue, Oct 23, 2012 at 02:09:39PM -0700, Andrew Morton wrote:
> On Tue, 23 Oct 2012 22:49:26 +0200
> Borislav Petkov <bp@...64.org> wrote:
> 
> > Now you only need to give me a proper patch with your S-O-B and we're
> > ready to go :).
> 
> who, me, what?!?!  Sounds stressful.

Yeah, this is to show you how we all feel when we're on the sending end
of patches :-).

> 
> umm, OK here we go.
> 
> What I don't understand is the effects of the bug.  If the present code
> can indeed start using the n+1th element of the array then it's writing
> random garbage into the hardware.  If this can ever happen then I
> suspect that yes, a cc:stable is needed?

Yes, cc:stable is needed.

The code has a very subtle bug because I should've robustified it from
the get-go instead of fiddling with it.

Basically, this is what happens:

If you supply new_bw < 761, i.e. so that the last element matches, *and*
you have a minimum scrubrate set to != 0x0 (due to unsupported scrubrate
settings on some families) the first continue in the loop with i being
22 (last array element) terminates the loop after incrementing i one
last time.

Thus, it doesn't let us break out of it in the next statement where
the new_bw will match and we'll be able to exit the loop without it
incrementing i one last time and thus get us out of bounds.

So yes, your fix is correct, I'll test it tomorrow just in case and
will sit down and rewrite this loop because it pisses me off to see how
stupidly fragile it is.

And I see you'll carry this patch so I won't send it to Linus next merge
window so thanks for this!

> From: Andrew Morton <akpm@...ux-foundation.org>
> Subject: drivers/edac/amd64_edac.c:__amd64_set_scrub_rate(): avoid overindexing scrubrates[]
> 
> If none of the elements in scrubrates[] matches, this loop will cause
> __amd64_set_scrub_rate() to incorrectly use the n+1th element.
> 
> As the function is designed to use the final scrubrates[] element in the
> case of no match, we can fix this bug by simply terminating the array
> search at the n-1th element.
> 
> Reported-by: Denis Kirjanov <kirjanov@...il.com>
> Cc: Doug Thompson <dougthompson@...ssion.com>
> Cc: Borislav Petkov <borislav.petkov@....com>

Acked-by: Borislav Petkov <borislav.petkov@....com>
Cc: stable@...r.kernel.org

> Signed-off-by: Andrew Morton <akpm@...ux-foundation.org>

[ … ]

-- 
Regards/Gruss,
Boris.

Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
GM: Alberto Bozzo
Reg: Dornach, Landkreis Muenchen
HRB Nr. 43632 WEEE Registernr: 129 19551
--
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/