| 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
| ||
|
Message-ID: <20110205173449.GA31845@liondog.tnic>
Date: Sat, 5 Feb 2011 18:34:49 +0100
From: Borislav Petkov <bp@...en8.de>
To: Alexey Dobriyan <adobriyan@...il.com>
Cc: akpm@...ux-foundation.org, linux-kernel@...r.kernel.org,
borislav.petkov@....com
Subject: Re: [PATCH 16/52] kstrtox: convert drivers/edac/
On Sat, Feb 05, 2011 at 04:20:19PM +0200, Alexey Dobriyan wrote:
Some kind of a boilerplate commit message is still better than none at all.
>
> Signed-off-by: Alexey Dobriyan <adobriyan@...il.com>
> ---
> drivers/edac/amd64_edac_inj.c | 135 +++++++++++++++++------------------------
> drivers/edac/edac_mc_sysfs.c | 20 +++---
> drivers/edac/i7core_edac.c | 41 +++++++------
> drivers/edac/mce_amd_inj.c | 28 ++++-----
> 4 files changed, 101 insertions(+), 123 deletions(-)
>
> diff --git a/drivers/edac/amd64_edac_inj.c b/drivers/edac/amd64_edac_inj.c
> index 688478d..180a498 100644
> --- a/drivers/edac/amd64_edac_inj.c
> +++ b/drivers/edac/amd64_edac_inj.c
> @@ -16,21 +16,18 @@ static ssize_t amd64_inject_section_store(struct mem_ctl_info *mci,
> const char *data, size_t count)
> {
> struct amd64_pvt *pvt = mci->pvt_info;
> - unsigned long value;
> - int ret = 0;
> -
> - ret = strict_strtoul(data, 10, &value);
> - if (ret != -EINVAL) {
> -
> - if (value > 3) {
> - amd64_warn("%s: invalid section 0x%lx\n", __func__, value);
> - return -EINVAL;
> - }
> -
> - pvt->injection.section = (u32) value;
> - return count;
> + u32 value;
> + int ret;
> +
> + ret = kstrtou32(data, 10, &value);
> + if (ret < 0)
> + return ret;
newline here
> + if (value > 3) {
> + amd64_warn("%s: invalid section 0x%x\n", __func__, value);
> + return -EINVAL;
> }
and here.
> - return ret;
> + pvt->injection.section = value;
> + return count;
> }
>
> static ssize_t amd64_inject_word_show(struct mem_ctl_info *mci, char *buf)
> @@ -49,21 +46,18 @@ static ssize_t amd64_inject_word_store(struct mem_ctl_info *mci,
> const char *data, size_t count)
> {
> struct amd64_pvt *pvt = mci->pvt_info;
> - unsigned long value;
> - int ret = 0;
> -
> - ret = strict_strtoul(data, 10, &value);
> - if (ret != -EINVAL) {
> -
> - if (value > 8) {
> - amd64_warn("%s: invalid word 0x%lx\n", __func__, value);
> - return -EINVAL;
> - }
> -
> - pvt->injection.word = (u32) value;
> - return count;
> + u32 value;
> + int ret;
> +
> + ret = kstrtou32(data, 10, &value);
> + if (ret < 0)
> + return ret;
newline
> + if (value > 8) {
> + amd64_warn("%s: invalid word 0x%x\n", __func__, value);
> + return -EINVAL;
> }
newline.
> - return ret;
> + pvt->injection.word = value;
> + return count;
> }
>
> static ssize_t amd64_inject_ecc_vector_show(struct mem_ctl_info *mci, char *buf)
> @@ -81,22 +75,19 @@ static ssize_t amd64_inject_ecc_vector_store(struct mem_ctl_info *mci,
> const char *data, size_t count)
> {
> struct amd64_pvt *pvt = mci->pvt_info;
> - unsigned long value;
> - int ret = 0;
> -
> - ret = strict_strtoul(data, 16, &value);
> - if (ret != -EINVAL) {
> -
> - if (value & 0xFFFF0000) {
> - amd64_warn("%s: invalid EccVector: 0x%lx\n",
> + u32 value;
> + int ret;
> +
> + ret = kstrtou32(data, 16, &value);
> + if (ret < 0)
> + return ret;
newline.
> + if (value & 0xFFFF0000) {
> + amd64_warn("%s: invalid EccVector: 0x%x\n",
> __func__, value);
> - return -EINVAL;
> - }
> -
> - pvt->injection.bit_map = (u32) value;
> - return count;
> + return -EINVAL;
> }
> - return ret;
newline
> + pvt->injection.bit_map = value;
> + return count;
> }
>
> /*
> @@ -107,29 +98,22 @@ static ssize_t amd64_inject_read_store(struct mem_ctl_info *mci,
> const char *data, size_t count)
> {
> struct amd64_pvt *pvt = mci->pvt_info;
> - unsigned long value;
> u32 section, word_bits;
> - int ret = 0;
> -
> - ret = strict_strtoul(data, 10, &value);
> - if (ret != -EINVAL) {
Dropping those would mean that the injection read (and write) below
would happen on any value written. Let's keep them and enforce a write
of '1' to be only valid injection trigger like the rest of the /sysfs
interfaces:
ret = kstrtou8(data, 10, &value);
if (ret < 0)
return ret;
if (value != 1)
return -EINVAL;
>
> - /* Form value to choose 16-byte section of cacheline */
> - section = F10_NB_ARRAY_DRAM_ECC |
> - SET_NB_ARRAY_ADDRESS(pvt->injection.section);
> - pci_write_config_dword(pvt->F3, F10_NB_ARRAY_ADDR, section);
> + /* Form value to choose 16-byte section of cacheline */
> + section = F10_NB_ARRAY_DRAM_ECC |
> + SET_NB_ARRAY_ADDRESS(pvt->injection.section);
> + pci_write_config_dword(pvt->F3, F10_NB_ARRAY_ADDR, section);
>
> - word_bits = SET_NB_DRAM_INJECTION_READ(pvt->injection.word,
> - pvt->injection.bit_map);
> + word_bits = SET_NB_DRAM_INJECTION_READ(pvt->injection.word,
> + pvt->injection.bit_map);
>
> - /* Issue 'word' and 'bit' along with the READ request */
> - pci_write_config_dword(pvt->F3, F10_NB_ARRAY_DATA, word_bits);
> + /* Issue 'word' and 'bit' along with the READ request */
> + pci_write_config_dword(pvt->F3, F10_NB_ARRAY_DATA, word_bits);
>
> - debugf0("section=0x%x word_bits=0x%x\n", section, word_bits);
> + debugf0("section=0x%x word_bits=0x%x\n", section, word_bits);
>
> - return count;
> - }
> - return ret;
> + return count;
> }
>
> /*
> @@ -140,29 +124,22 @@ static ssize_t amd64_inject_write_store(struct mem_ctl_info *mci,
> const char *data, size_t count)
> {
> struct amd64_pvt *pvt = mci->pvt_info;
> - unsigned long value;
> u32 section, word_bits;
> - int ret = 0;
> -
> - ret = strict_strtoul(data, 10, &value);
> - if (ret != -EINVAL) {
Same as above.
>
> - /* Form value to choose 16-byte section of cacheline */
> - section = F10_NB_ARRAY_DRAM_ECC |
> - SET_NB_ARRAY_ADDRESS(pvt->injection.section);
> - pci_write_config_dword(pvt->F3, F10_NB_ARRAY_ADDR, section);
> + /* Form value to choose 16-byte section of cacheline */
> + section = F10_NB_ARRAY_DRAM_ECC |
> + SET_NB_ARRAY_ADDRESS(pvt->injection.section);
> + pci_write_config_dword(pvt->F3, F10_NB_ARRAY_ADDR, section);
>
> - word_bits = SET_NB_DRAM_INJECTION_WRITE(pvt->injection.word,
> - pvt->injection.bit_map);
> + word_bits = SET_NB_DRAM_INJECTION_WRITE(pvt->injection.word,
> + pvt->injection.bit_map);
>
> - /* Issue 'word' and 'bit' along with the READ request */
> - pci_write_config_dword(pvt->F3, F10_NB_ARRAY_DATA, word_bits);
> + /* Issue 'word' and 'bit' along with the READ request */
> + pci_write_config_dword(pvt->F3, F10_NB_ARRAY_DATA, word_bits);
>
> - debugf0("section=0x%x word_bits=0x%x\n", section, word_bits);
> + debugf0("section=0x%x word_bits=0x%x\n", section, word_bits);
>
> - return count;
> - }
> - return ret;
> + return count;
> }
>
> /*
> @@ -197,17 +174,15 @@ struct mcidev_sysfs_attribute amd64_inj_attrs[] = {
> {
> .attr = {
> .name = "inject_write",
> - .mode = (S_IRUGO | S_IWUSR)
> + .mode = S_IWUSR,
This change is not mentioned anywhere, why do we need it?
> },
> - .show = NULL,
> .store = amd64_inject_write_store,
> },
> {
> .attr = {
> .name = "inject_read",
> - .mode = (S_IRUGO | S_IWUSR)
> + .mode = S_IWUSR,
ditto.
> },
> - .show = NULL,
> .store = amd64_inject_read_store,
> },
> };
Ok, provided the general change to kstrtoX is agreed upon, you can add
my Acked-by to the AMD EDAC changes pending the issues above.
Thanks.
--
Regards/Gruss,
Boris.
--
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/
Powered by blists - more mailing lists