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

Andrew Morton
2012-10-23T20:37:16Z

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?