* [PATCH] edac: fix buffer overrun if no suitable bandwidth found @ 2012-10-22 15:30 Denis Kirjanov 2012-10-23 15:39 ` Borislav Petkov 2012-10-23 19:10 ` Andrew Morton 0 siblings, 2 replies; 9+ messages in thread From: Denis Kirjanov @ 2012-10-22 15:30 UTC (permalink / raw) To: linux-edac; +Cc: linux-kernel, stable, Denis Kirjanov fix buffer overrun if no suitable bandwidth found Signed-off-by: Denis Kirjanov <kirjanov@gmail.com> --- drivers/edac/amd64_edac.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c index 5a297a2..d85ad9e 100644 --- a/drivers/edac/amd64_edac.c +++ b/drivers/edac/amd64_edac.c @@ -188,6 +188,9 @@ static int __amd64_set_scrub_rate(struct pci_dev *ctl, u32 new_bw, u32 min_rate) * scrubrates array. */ } + if (i == ARRAY_SIZE(scrubrates)) { + i--; + } scrubval = scrubrates[i].scrubval; -- 1.7.9.5 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] edac: fix buffer overrun if no suitable bandwidth found 2012-10-22 15:30 [PATCH] edac: fix buffer overrun if no suitable bandwidth found Denis Kirjanov @ 2012-10-23 15:39 ` Borislav Petkov 2012-10-23 19:10 ` Andrew Morton 1 sibling, 0 replies; 9+ messages in thread From: Borislav Petkov @ 2012-10-23 15:39 UTC (permalink / raw) To: Denis Kirjanov; +Cc: linux-edac, linux-kernel On Mon, Oct 22, 2012 at 07:30:58PM +0400, Denis Kirjanov wrote: > fix buffer overrun if no suitable bandwidth found > > Signed-off-by: Denis Kirjanov <kirjanov@gmail.com> Applied with modifications and stable dropped. Thanks. -- 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] edac: fix buffer overrun if no suitable bandwidth found 2012-10-22 15:30 [PATCH] edac: fix buffer overrun if no suitable bandwidth found Denis Kirjanov 2012-10-23 15:39 ` Borislav Petkov @ 2012-10-23 19:10 ` Andrew Morton 2012-10-23 20:26 ` Borislav Petkov 1 sibling, 1 reply; 9+ messages in thread From: Andrew Morton @ 2012-10-23 19:10 UTC (permalink / raw) To: Denis Kirjanov Cc: linux-edac, linux-kernel, stable, Doug Thompson, Borislav Petkov On Mon, 22 Oct 2012 19:30:58 +0400 Denis Kirjanov <kirjanov@gmail.com> wrote: > fix buffer overrun if no suitable bandwidth found > > Signed-off-by: Denis Kirjanov <kirjanov@gmail.com> > --- > drivers/edac/amd64_edac.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c > index 5a297a2..d85ad9e 100644 > --- a/drivers/edac/amd64_edac.c > +++ b/drivers/edac/amd64_edac.c > @@ -188,6 +188,9 @@ static int __amd64_set_scrub_rate(struct pci_dev *ctl, u32 new_bw, u32 min_rate) > * scrubrates array. > */ > } > + if (i == ARRAY_SIZE(scrubrates)) { > + i--; > + } > > scrubval = scrubrates[i].scrubval; > That's pretty strange code in there. If the comment is to be believed, isn't this a suitable fix? --- a/drivers/edac/amd64_edac.c~a +++ a/drivers/edac/amd64_edac.c @@ -171,7 +171,7 @@ static int __amd64_set_scrub_rate(struct * bandwidth entry that is greater or equal than the setting requested * and program that. If at last entry, turn off DRAM scrubbing. */ - for (i = 0; i < ARRAY_SIZE(scrubrates); i++) { + for (i = 0; i < ARRAY_SIZE(scrubrates) - 1; i++) { /* * skip scrub rates which aren't recommended * (see F10 BKDG, F3x58) _ Also, I don't think "buffer overrun" is an appropriate description here - to me, "buffer overrun" implies writing to memory outside the buffer. I'd call this "array overindexing" or similar. Finally, when fixing a bug, please always describe the user-visible impact of that bug. You have cc'ed stable on this patch (using the incorrect email address, btw) which implies that the effects are serious, but people will want to know specific details about those effects when considering the patch. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] edac: fix buffer overrun if no suitable bandwidth found 2012-10-23 19:10 ` Andrew Morton @ 2012-10-23 20:26 ` Borislav Petkov 2012-10-23 20:37 ` Andrew Morton 0 siblings, 1 reply; 9+ messages in thread From: Borislav Petkov @ 2012-10-23 20:26 UTC (permalink / raw) To: Andrew Morton Cc: Denis Kirjanov, linux-edac, linux-kernel, stable, Doug Thompson, Borislav Petkov On Tue, Oct 23, 2012 at 12:10:05PM -0700, Andrew Morton wrote: > That's pretty strange code in there. > > If the comment is to be believed, isn't this a suitable fix? > > --- a/drivers/edac/amd64_edac.c~a > +++ a/drivers/edac/amd64_edac.c > @@ -171,7 +171,7 @@ static int __amd64_set_scrub_rate(struct > * bandwidth entry that is greater or equal than the setting requested > * and program that. If at last entry, turn off DRAM scrubbing. > */ > - for (i = 0; i < ARRAY_SIZE(scrubrates); i++) { > + for (i = 0; i < ARRAY_SIZE(scrubrates) - 1; i++) { > /* > * skip scrub rates which aren't recommended > * (see F10 BKDG, F3x58) > _ > > Also, I don't think "buffer overrun" is an appropriate description here > - to me, "buffer overrun" implies writing to memory outside the buffer. > I'd call this "array overindexing" or similar. > > Finally, when fixing a bug, please always describe the user-visible > impact of that bug. You have cc'ed stable on this patch (using the > incorrect email address, btw) which implies that the effects are serious, > but people will want to know specific details about those effects when > considering the patch. Right, I took it for correctness' sake but after a heavy massaging. And this is only a hypothetical case since we're always falling back to the last element of scrubrates array which turns off scrubbing, based on the supplied bandwidth. Thus no need for stable, IMO. Let me know if this is what you had in mind. [ And yes, maybe I should rewrite this to de-awkwardize it :) ] -- >From 70f063eb9aa9674613a21fb8e3f21ec4df0629c7 Mon Sep 17 00:00:00 2001 From: Denis Kirjanov <kirjanov@gmail.com> Date: Mon, 22 Oct 2012 19:30:58 +0400 Subject: [PATCH] amd64_edac: Fix hypothetical out-of-bounds access Make sure we stay within scrubrates' array bounds. Boris: this is a correctness fix only because the loop terminates earlier due to us capping scrubbing bandwidth to 0. Signed-off-by: Denis Kirjanov <kirjanov@gmail.com> Signed-off-by: Borislav Petkov <borislav.petkov@amd.com> --- drivers/edac/amd64_edac.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c index 501bfb938f26..73d9108d6200 100644 --- a/drivers/edac/amd64_edac.c +++ b/drivers/edac/amd64_edac.c @@ -181,14 +181,16 @@ static int __amd64_set_scrub_rate(struct pci_dev *ctl, u32 new_bw, u32 min_rate) if (scrubrates[i].bandwidth <= new_bw) break; - - /* - * if no suitable bandwidth found, turn off DRAM scrubbing - * entirely by falling back to the last element in the - * scrubrates array. - */ } + /* + * if no suitable bandwidth found, turn off DRAM scrubbing + * entirely by falling back to the last element in the scrubrates + * array. + */ + if (i == ARRAY_SIZE(scrubrates)) + i--; + scrubval = scrubrates[i].scrubval; pci_write_bits32(ctl, SCRCTRL, scrubval, 0x001F); -- 1.8.0 Thanks. -- 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] edac: fix buffer overrun if no suitable bandwidth found 2012-10-23 20:26 ` Borislav Petkov @ 2012-10-23 20:37 ` Andrew Morton 2012-10-23 20:49 ` Borislav Petkov 0 siblings, 1 reply; 9+ messages in thread From: Andrew Morton @ 2012-10-23 20:37 UTC (permalink / raw) To: Borislav Petkov Cc: Denis Kirjanov, linux-edac, linux-kernel, stable, Doug Thompson, Borislav Petkov On Tue, 23 Oct 2012 22:26:12 +0200 Borislav Petkov <bp@amd64.org> wrote: > From: Denis Kirjanov <kirjanov@gmail.com> > Date: Mon, 22 Oct 2012 19:30:58 +0400 > Subject: [PATCH] amd64_edac: Fix hypothetical out-of-bounds access > > Make sure we stay within scrubrates' array bounds. > > Boris: this is a correctness fix only because the loop terminates > earlier due to us capping scrubbing bandwidth to 0. > > Signed-off-by: Denis Kirjanov <kirjanov@gmail.com> > Signed-off-by: Borislav Petkov <borislav.petkov@amd.com> > --- > drivers/edac/amd64_edac.c | 14 ++++++++------ > 1 file changed, 8 insertions(+), 6 deletions(-) > > diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c > index 501bfb938f26..73d9108d6200 100644 > --- a/drivers/edac/amd64_edac.c > +++ b/drivers/edac/amd64_edac.c > @@ -181,14 +181,16 @@ static int __amd64_set_scrub_rate(struct pci_dev *ctl, u32 new_bw, u32 min_rate) > > if (scrubrates[i].bandwidth <= new_bw) > break; > - > - /* > - * if no suitable bandwidth found, turn off DRAM scrubbing > - * entirely by falling back to the last element in the > - * scrubrates array. > - */ > } > > + /* > + * if no suitable bandwidth found, turn off DRAM scrubbing > + * entirely by falling back to the last element in the scrubrates > + * array. > + */ > + if (i == ARRAY_SIZE(scrubrates)) > + i--; > + > scrubval = scrubrates[i].scrubval; > > pci_write_bits32(ctl, SCRCTRL, scrubval, 0x001F); This is still strange. What's the point in having the initial loop even consider the last element in the array if we know we'll be using it anyway? ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] edac: fix buffer overrun if no suitable bandwidth found 2012-10-23 20:37 ` Andrew Morton @ 2012-10-23 20:49 ` Borislav Petkov 2012-10-23 21:09 ` Andrew Morton 0 siblings, 1 reply; 9+ messages in thread From: Borislav Petkov @ 2012-10-23 20:49 UTC (permalink / raw) To: Andrew Morton; +Cc: Denis Kirjanov, linux-edac, linux-kernel, stable On Tue, Oct 23, 2012 at 01:37:09PM -0700, Andrew Morton wrote: > This is still strange. What's the point in having the initial loop > even consider the last element in the array if we know we'll be using > it anyway? You're right, yours is better. Now you only need to give me a proper patch with your S-O-B and we're ready to go :). Thanks. -- 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] edac: fix buffer overrun if no suitable bandwidth found 2012-10-23 20:49 ` Borislav Petkov @ 2012-10-23 21:09 ` Andrew Morton 2012-10-23 21:38 ` Borislav Petkov 0 siblings, 1 reply; 9+ messages in thread From: Andrew Morton @ 2012-10-23 21:09 UTC (permalink / raw) To: Borislav Petkov; +Cc: Denis Kirjanov, linux-edac, linux-kernel, stable On Tue, 23 Oct 2012 22:49:26 +0200 Borislav Petkov <bp@amd64.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. 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? From: Andrew Morton <akpm@linux-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@gmail.com> Cc: Doug Thompson <dougthompson@xmission.com> Cc: Borislav Petkov <borislav.petkov@amd.com> Signed-off-by: Andrew Morton <akpm@linux-foundation.org> --- drivers/edac/amd64_edac.c | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff -puN drivers/edac/amd64_edac.c~drivers-edac-amd64_edacc-__amd64_set_scrub_rate-avoid-overindexing-scrubrates drivers/edac/amd64_edac.c --- a/drivers/edac/amd64_edac.c~drivers-edac-amd64_edacc-__amd64_set_scrub_rate-avoid-overindexing-scrubrates +++ a/drivers/edac/amd64_edac.c @@ -170,8 +170,11 @@ static int __amd64_set_scrub_rate(struct * memory controller and apply to register. Search for the first * bandwidth entry that is greater or equal than the setting requested * and program that. If at last entry, turn off DRAM scrubbing. + * + * If no suitable bandwidth is found, turn off DRAM scrubbing entirely + * by falling back to the last element in scrubrates[]. */ - for (i = 0; i < ARRAY_SIZE(scrubrates); i++) { + for (i = 0; i < ARRAY_SIZE(scrubrates) - 1; i++) { /* * skip scrub rates which aren't recommended * (see F10 BKDG, F3x58) @@ -181,12 +184,6 @@ static int __amd64_set_scrub_rate(struct if (scrubrates[i].bandwidth <= new_bw) break; - - /* - * if no suitable bandwidth found, turn off DRAM scrubbing - * entirely by falling back to the last element in the - * scrubrates array. - */ } scrubval = scrubrates[i].scrubval; _ ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] edac: fix buffer overrun if no suitable bandwidth found 2012-10-23 21:09 ` Andrew Morton @ 2012-10-23 21:38 ` Borislav Petkov 2012-10-23 21:58 ` Andrew Morton 0 siblings, 1 reply; 9+ messages in thread From: Borislav Petkov @ 2012-10-23 21:38 UTC (permalink / raw) To: Andrew Morton; +Cc: Denis Kirjanov, linux-edac, linux-kernel, stable 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@amd64.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@linux-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@gmail.com> > Cc: Doug Thompson <dougthompson@xmission.com> > Cc: Borislav Petkov <borislav.petkov@amd.com> Acked-by: Borislav Petkov <borislav.petkov@amd.com> Cc: stable@vger.kernel.org > Signed-off-by: Andrew Morton <akpm@linux-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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] edac: fix buffer overrun if no suitable bandwidth found 2012-10-23 21:38 ` Borislav Petkov @ 2012-10-23 21:58 ` Andrew Morton 0 siblings, 0 replies; 9+ messages in thread From: Andrew Morton @ 2012-10-23 21:58 UTC (permalink / raw) To: Borislav Petkov; +Cc: Denis Kirjanov, linux-edac, linux-kernel, stable On Tue, 23 Oct 2012 23:38:45 +0200 Borislav Petkov <bp@amd64.org> wrote: > And I see you'll carry this patch so I won't send it to Linus next merge > window so thanks for this! No, please merge it yourself. Once it (or a version of it) turns up in linux-next, I'll drop my copy. Merging stuff I shouldn't is my crude way of keeping track of things ;) ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2012-10-23 21:58 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-10-22 15:30 [PATCH] edac: fix buffer overrun if no suitable bandwidth found Denis Kirjanov 2012-10-23 15:39 ` Borislav Petkov 2012-10-23 19:10 ` Andrew Morton 2012-10-23 20:26 ` Borislav Petkov 2012-10-23 20:37 ` Andrew Morton 2012-10-23 20:49 ` Borislav Petkov 2012-10-23 21:09 ` Andrew Morton 2012-10-23 21:38 ` Borislav Petkov 2012-10-23 21:58 ` Andrew Morton
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox