linux-kernel - Re: [PATCH] firmware: google: update vpd

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <20190807135834.GA12853@roeck-us.net>
Date:   Wed, 7 Aug 2019 06:58:34 -0700
From:   Guenter Roeck <linux@...ck-us.net>
To:     Stephen Boyd <swboyd@...omium.org>
Cc:     Hung-Te Lin <hungte@...omium.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Anton Vasilyev <vasilyev@...ras.ru>,
        Colin Ian King <colin.king@...onical.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Alexios Zavras <alexios.zavras@...el.com>,
        Samuel Holland <samuel@...lland.org>,
        Allison Randal <allison@...utok.net>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] firmware: google: update vpd_decode from upstream

On Fri, Aug 02, 2019 at 03:27:54PM -0700, Stephen Boyd wrote:
> Quoting Hung-Te Lin (2019-08-02 01:20:31)
> > The VPD implementation from Chromium Vital Product Data project has been
> > updated so vpd_decode be easily shared by kernel, firmware and the user
> > space utility programs. Also improved value range checks to prevent
> > kernel crash due to bad VPD data.
> 
> Please add a Fixes: tag here to fix the commit that introduces the
> problem. It would also be nice to get a description of the problem that
> this patch is fixing. For example, explaining why the types change from
> signed to unsigned.
> 
> > 
> > Signed-off-by: Hung-Te Lin <hungte@...omium.org>
> > ---
> >  drivers/firmware/google/vpd.c        | 38 +++++++++------
> >  drivers/firmware/google/vpd_decode.c | 69 +++++++++++++++-------------
> >  drivers/firmware/google/vpd_decode.h | 17 ++++---
> >  3 files changed, 71 insertions(+), 53 deletions(-)
> > 
> > diff --git a/drivers/firmware/google/vpd.c b/drivers/firmware/google/vpd.c
> > index 0739f3b70347..ecf217a7db39 100644
> > --- a/drivers/firmware/google/vpd.c
> > +++ b/drivers/firmware/google/vpd.c
> > @@ -73,7 +73,7 @@ static ssize_t vpd_attrib_read(struct file *filp, struct kobject *kobp,
> >   * exporting them as sysfs attributes. These keys present in old firmwares are
> >   * ignored.
> >   *
> > - * Returns VPD_OK for a valid key name, VPD_FAIL otherwise.
> > + * Returns VPD_DECODE_OK for a valid key name, VPD_DECODE_FAIL otherwise.
> 
> Maybe we should convert these things to use linux conventions instead of
> VPD error codes?
> 
> >   *
> >   * @key: The key name to check
> >   * @key_len: key name length
> > @@ -86,14 +86,14 @@ static int vpd_section_check_key_name(const u8 *key, s32 key_len)
> >                 c = *key++;
> >  
> >                 if (!isalnum(c) && c != '_')
> > -                       return VPD_FAIL;
> > +                       return VPD_DECODE_FAIL;
> >         }
> >  
> > -       return VPD_OK;
> > +       return VPD_DECODE_OK;
> 
> Can you split this rename out into it's own patch. That way we can
> confirm that there are no changes due to the rename of the enum.
> 
> >  }
> >  
> > -static int vpd_section_attrib_add(const u8 *key, s32 key_len,
> > -                                 const u8 *value, s32 value_len,
> > +static int vpd_section_attrib_add(const u8 *key, u32 key_len,
> > +                                 const u8 *value, u32 value_len,
> >                                   void *arg)
> >  {
> >         int ret;
> > @@ -246,7 +246,7 @@ static int vpd_section_destroy(struct vpd_section *sec)
> >  
> >  static int vpd_sections_init(phys_addr_t physaddr)
> >  {
> > -       struct vpd_cbmem *temp;
> > +       struct vpd_cbmem __iomem *temp;

The change to __iomem should also be a separate patch.

Guenter