fix buffer overrun if no suitable bandwidth found
Signed-off-by: Denis Kirjanov <[email protected]>
---
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
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 <[email protected]>
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
On Mon, 22 Oct 2012 19:30:58 +0400
Denis Kirjanov <[email protected]> wrote:
> fix buffer overrun if no suitable bandwidth found
>
> Signed-off-by: Denis Kirjanov <[email protected]>
> ---
> 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.
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 <[email protected]>
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 <[email protected]>
Signed-off-by: Borislav Petkov <[email protected]>
---
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
On Tue, 23 Oct 2012 22:26:12 +0200
Borislav Petkov <[email protected]> wrote:
> From: Denis Kirjanov <[email protected]>
> 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 <[email protected]>
> Signed-off-by: Borislav Petkov <[email protected]>
> ---
> 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?
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
On Tue, 23 Oct 2012 22:49:26 +0200
Borislav Petkov <[email protected]> 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 <[email protected]>
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 <[email protected]>
Cc: Doug Thompson <[email protected]>
Cc: Borislav Petkov <[email protected]>
Signed-off-by: Andrew Morton <[email protected]>
---
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;
_
On Tue, Oct 23, 2012 at 02:09:39PM -0700, Andrew Morton wrote:
> On Tue, 23 Oct 2012 22:49:26 +0200
> Borislav Petkov <[email protected]> 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 <[email protected]>
> 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 <[email protected]>
> Cc: Doug Thompson <[email protected]>
> Cc: Borislav Petkov <[email protected]>
Acked-by: Borislav Petkov <[email protected]>
Cc: [email protected]
> Signed-off-by: Andrew Morton <[email protected]>
[ … ]
--
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
On Tue, 23 Oct 2012 23:38:45 +0200
Borislav Petkov <[email protected]> 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 ;)