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  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]
Date:   Fri, 17 Jul 2020 16:48:09 +0200
From:   Arnd Bergmann <arnd@...db.de>
To:     Daniel Gutson <daniel.gutson@...ypsium.com>
Cc:     Derek Kiernan <derek.kiernan@...inx.com>,
        Tudor Ambarus <tudor.ambarus@...rochip.com>,
        Miquel Raynal <miquel.raynal@...tlin.com>,
        Richard Weinberger <richard@....at>,
        Vignesh Raghavendra <vigneshr@...com>,
        Mika Westerberg <mika.westerberg@...ux.intel.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Mauro Carvalho Chehab <mchehab+huawei@...nel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Richard Hughes <hughsient@...il.com>,
        Alex Bazhaniuk <alex@...ypsium.com>
Subject: Re: [PATCH] [PATCH] Firmware security information in SYSFS

On Fri, Jul 17, 2020 at 12:36 AM Daniel Gutson
<daniel.gutson@...ypsium.com> wrote:
>

> +static ssize_t internal_callback(struct kobject *kobj,
> +                                struct kobj_attribute *attr, char *buf)
> +{
> +       struct firmware_security_data *fwsd =
> +               container_of(attr, struct firmware_security_data, kobj_attr);
> +       return fwsd->callback(buf, fwsd->private_data);
> +}
> +
> +int register_firmware_security_data_callback(
> +       const char *name, ssize_t (*callback)(char *buf, void *private_data),
> +       void *private_data)

Why do you have a callback interface here? I would have expected
all the data to be static during the kernel run time, so just passing the
information here would make it much simpler.

> +static ssize_t cnl_read_field(char *buf, struct pci_dev *pdev, u32 mask)
> +{
> +       u32 bcr;
> +
> +       if (pci_read_config_dword(pdev, BCR, &bcr) != PCIBIOS_SUCCESSFUL)
> +               return -EIO;
> +
> +       return sprintf(buf, "%d\n", (int)!!(bcr & mask));
> +}
> +
> +static ssize_t cnl_wpd_firmware_security_callbacks(char *buf,
> +                                                  void *private_data)
> +{
> +       return cnl_read_field(buf, (struct pci_dev *)private_data, BCR_WPD);
> +}
> +
> +static ssize_t cnl_ble_firmware_security_callbacks(char *buf,
> +                                                  void *private_data)
> +{
> +       return cnl_read_field(buf, (struct pci_dev *)private_data, BCR_BLE);
> +}
> +
> +static ssize_t cnl_smm_bwp_firmware_security_callbacks(char *buf,
> +                                                      void *private_data)
> +{
> +       return cnl_read_field(buf, (struct pci_dev *)private_data, BCR_SMM_BWP);
> +}
> +
> +static void register_firmware_security_data_callbacks(struct pci_dev *pdev)
> +{
> +       register_firmware_security_data_callback(
> +               "bioswe", &cnl_wpd_firmware_security_callbacks, pdev);
> +       register_firmware_security_data_callback(
> +               "ble", &cnl_ble_firmware_security_callbacks, pdev);
> +       register_firmware_security_data_callback(
> +               "smm_bwp", &cnl_smm_bwp_firmware_security_callbacks, pdev);
> +}

All of this should probably become a single function that reads the
registers and then passes the contents into whatever common function
you have that makes them visible to user space.

> @@ -76,7 +119,7 @@ static const struct pci_device_id intel_spi_pci_ids[] = {
>         { PCI_VDEVICE(INTEL, 0xa224), (unsigned long)&bxt_info },
>         { PCI_VDEVICE(INTEL, 0xa324), (unsigned long)&cnl_info },
>         { PCI_VDEVICE(INTEL, 0xa3a4), (unsigned long)&bxt_info },
> -       { },
> +       {},
>  };
...
>  #include "intel-spi.h"
>
> @@ -48,17 +49,17 @@
>
>  #define FADDR                          0x08
>  #define DLOCK                          0x0c
> -#define FDATA(n)                       (0x10 + ((n) * 4))
> +#define FDATA(n)                       (0x10 + ((n)*4))
>
>  #define FRACC                          0x50
>
> -#define FREG(n)                                (0x54 + ((n) * 4))
> +#define FREG(n)                                (0x54 + ((n)*4))
>  #define FREG_BASE_MASK                 0x3fff
>  #define FREG_LIMIT_SHIFT               16
>  #define FREG_LIMIT_MASK                        (0x03fff << FREG_LIMIT_SHIFT)
>

Accidental whitespace change, please skip these changes?

> @@ -161,7 +164,8 @@ struct intel_spi {
>
>  static bool writeable;
>  module_param(writeable, bool, 0);
> -MODULE_PARM_DESC(writeable, "Enable write access to SPI flash chip (default=0)");
> +MODULE_PARM_DESC(writeable,
> +                "Enable write access to SPI flash chip (default=0)");
>
>  static void intel_spi_dump_regs(struct intel_spi *ispi)
>  {

This looks like a useful change, but it should be a separate patch.

> @@ -179,8 +183,8 @@ static void intel_spi_dump_regs(struct intel_spi *ispi)
>         dev_dbg(ispi->dev, "DLOCK=0x%08x\n", readl(ispi->base + DLOCK));
>
>         for (i = 0; i < 16; i++)
> -               dev_dbg(ispi->dev, "FDATA(%d)=0x%08x\n",
> -                       i, readl(ispi->base + FDATA(i)));
> +               dev_dbg(ispi->dev, "FDATA(%d)=0x%08x\n", i,
> +                       readl(ispi->base + FDATA(i)));
>
>         dev_dbg(ispi->dev, "FRACC=0x%08x\n", readl(ispi->base + FRACC));
>
> @@ -220,9 +224,8 @@ static void intel_spi_dump_regs(struct intel_spi *ispi)
>                 base = value & PR_BASE_MASK;
>
>                 dev_dbg(ispi->dev, " %02d base: 0x%08x limit: 0x%08x [%c%c]\n",
> -                        i, base << 12, (limit << 12) | 0xfff,
> -                        value & PR_WPE ? 'W' : '.',
> -                        value & PR_RPE ? 'R' : '.');
> +                       i, base << 12, (limit << 12) | 0xfff,
> +                       value & PR_WPE ? 'W' : '.', value & PR_RPE ? 'R' : '.');
>         }
>
>         dev_dbg(ispi->dev, "Flash regions:\n");

More whitespace changes.

       Arnd

Powered by blists - more mailing lists