linux-kernel - Re: [PATCH] platform/x86: intel_pmc_core: export platform global

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

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 2 Apr 2021 14:12:20 +0300
From:   Andy Shevchenko <andy.shevchenko@...il.com>
To:     Tomas Winkler <tomas.winkler@...el.com>
Cc:     Rajneesh Bhardwaj <irenic.rajneesh@...il.com>,
        David E Box <david.e.box@...el.com>,
        Hans de Goede <hdegoede@...hat.com>,
        Mark Gross <mgross@...ux.intel.com>,
        Platform Driver <platform-driver-x86@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Tamar Mashiah <tamar.mashiah@...el.com>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Subject: Re: [PATCH] platform/x86: intel_pmc_core: export platform
 global_reset via sysfs.

On Fri, Apr 2, 2021 at 12:32 AM Tomas Winkler <tomas.winkler@...el.com> wrote:
>
> From: Tamar Mashiah <tamar.mashiah@...el.com>
>
> During PCH manufacturing a global reset has to be induced in order
> for configuration changes take affect upon following platform reset.

effect upon the following ?

> This setting was commonly done by accessing PMC registers via /dev/mem
> but due to security concern /dev/mem access is much restricted, hence
> the reason for exposing this setting via dedicated sysfs interface.
> To prevent post manufacturing abuse the register is protected
> by hardware locking.
>
> The register in MMIO space is defined for Cannon Lake and newer PCHs.
>
> Cc: David E Box <david.e.box@...el.com>

> Reviewed-by: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>

Hmm... okay, I forgot this, so my additional comments above and below.

...

> +static int set_global_reset(struct pmc_dev *pmcdev)
> +{
> +       const struct pmc_reg_map *map = pmcdev->map;
> +       u32 reg;
> +       int err;
> +
> +       mutex_lock(&pmcdev->lock);

> +       if (!map->etr3_offset) {
> +               err = -EOPNOTSUPP;
> +               goto out_unlock;
> +       }

Do we really need this check under the lock?

> +       /* check if CF9 is locked */
> +       reg = pmc_core_reg_read(pmcdev, map->etr3_offset);
> +       if (reg & ETR3_CF9LOCK) {
> +               err = -EACCES;
> +               goto out_unlock;
> +       }
> +
> +       /* write CF9 global reset bit */

Somewhere you use cf9 (small letters) I suggest to be consistent and
use the capitalized version everywhere.

> +       reg |= ETR3_CF9GR;
> +       pmc_core_reg_write(pmcdev, map->etr3_offset, reg);
> +
> +       reg = pmc_core_reg_read(pmcdev, map->etr3_offset);
> +       if ((reg & ETR3_CF9GR) == 0) {

Can be written in a form of !(reg & ETR3_CF9GR).

> +               err = -EIO;
> +               goto out_unlock;
> +       }
> +
> +       err = 0;
> +
> +out_unlock:
> +       mutex_unlock(&pmcdev->lock);
> +       return err;
> +}
> +
> +static ssize_t global_reset_show(struct device *dev,
> +                                struct device_attribute *attr, char *buf)
> +{
> +       struct pmc_dev *pmcdev = dev_get_drvdata(dev);
> +       const struct pmc_reg_map *map = pmcdev->map;
> +       u32 reg;
> +
> +       if (!map->etr3_offset)
> +               return -EOPNOTSUPP;

> +       reg = pmc_core_reg_read(pmcdev, map->etr3_offset);
> +       reg &= ETR3_CF9GR | ETR3_CF9LOCK;

And why no lock here?

> +       return sysfs_emit(buf, "0x%08x", reg);
> +}


-- 
With Best Regards,
Andy Shevchenko