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]
Message-ID: <CAHp75VcHjGAVog31AHaVLAq452=h=tqEN-1GNm_Aiu3113oTLA@mail.gmail.com>
Date:   Tue, 4 Jan 2022 13:26:28 +0200
From:   Andy Shevchenko <andy.shevchenko@...il.com>
To:     Hector Martin <marcan@...can.st>
Cc:     Kalle Valo <kvalo@...eaurora.org>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        "Rafael J. Wysocki" <rafael@...nel.org>,
        Len Brown <lenb@...nel.org>,
        Arend van Spriel <aspriel@...il.com>,
        Franky Lin <franky.lin@...adcom.com>,
        Hante Meuleman <hante.meuleman@...adcom.com>,
        Chi-hsien Lin <chi-hsien.lin@...ineon.com>,
        Wright Feng <wright.feng@...ineon.com>,
        Dmitry Osipenko <digetx@...il.com>,
        Sven Peter <sven@...npeter.dev>,
        Alyssa Rosenzweig <alyssa@...enzweig.io>,
        Mark Kettenis <kettenis@...nbsd.org>,
        Rafał Miłecki <zajec5@...il.com>,
        Pieter-Paul Giesberts <pieter-paul.giesberts@...adcom.com>,
        Linus Walleij <linus.walleij@...aro.org>,
        Hans de Goede <hdegoede@...hat.com>,
        "John W. Linville" <linville@...driver.com>,
        "brian m. carlson" <sandals@...stytoothpaste.net>,
        "open list:TI WILINK WIRELES..." <linux-wireless@...r.kernel.org>,
        netdev <netdev@...r.kernel.org>,
        devicetree <devicetree@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        ACPI Devel Maling List <linux-acpi@...r.kernel.org>,
        "open list:BROADCOM BRCM80211 IEEE802.11n WIRELESS DRIVER" 
        <brcm80211-dev-list.pdl@...adcom.com>,
        SHA-cyfmac-dev-list@...ineon.com
Subject: Re: [PATCH v2 07/35] brcmfmac: pcie: Read Apple OTP information

On Tue, Jan 4, 2022 at 9:28 AM Hector Martin <marcan@...can.st> wrote:
>
> On Apple platforms, the One Time Programmable ROM in the Broadcom chips
> contains information about the specific board design (module, vendor,
> version) that is required to select the correct NVRAM file. Parse this
> OTP ROM and extract the required strings.
>
> Note that the user OTP offset/size is per-chip. This patch does not add
> any chips yet.

...

> +static int
> +brcmf_pcie_parse_otp_sys_vendor(struct brcmf_pciedev_info *devinfo,
> +                               u8 *data, size_t size)
> +{
> +       int idx = 4;

Can you rather have a structure

struct my_cool_and_strange_blob {
__le32 hdr;
const char ...[];
...
}

and then cast your data to this struct?

> +       const char *chip_params;
> +       const char *board_params;
> +       const char *p;
> +
> +       /* 4-byte header and two empty strings */
> +       if (size < 6)
> +               return -EINVAL;
> +
> +       if (get_unaligned_le32(data) != BRCMF_OTP_VENDOR_HDR)
> +               return -EINVAL;
> +
> +       chip_params = &data[idx];

> +       /* Skip first string, including terminator */
> +       idx += strnlen(chip_params, size - idx) + 1;

strsep() ?

> +       if (idx >= size)
> +               return -EINVAL;
> +
> +       board_params = &data[idx];
> +
> +       /* Skip to terminator of second string */
> +       idx += strnlen(board_params, size - idx);
> +       if (idx >= size)
> +               return -EINVAL;
> +
> +       /* At this point both strings are guaranteed NUL-terminated */
> +       brcmf_dbg(PCIE, "OTP: chip_params='%s' board_params='%s'\n",
> +                 chip_params, board_params);
> +
> +       p = board_params;
> +       while (*p) {
> +               char tag = *p++;
> +               const char *end;
> +               size_t len;
> +
> +               if (tag == ' ') /* Skip extra spaces */
> +                       continue;

skip_spaces()

> +
> +               if (*p++ != '=') /* implicit NUL check */
> +                       return -EINVAL;

Have you checked the next_arg() implementation?

> +               /* *p might be NUL here, if so end == p and len == 0 */
> +               end = strchrnul(p, ' ');
> +               len = end - p;
> +
> +               /* leave 1 byte for NUL in destination string */
> +               if (len > (BRCMF_OTP_MAX_PARAM_LEN - 1))
> +                       return -EINVAL;
> +
> +               /* Copy len characters plus a NUL terminator */
> +               switch (tag) {
> +               case 'M':
> +                       strscpy(devinfo->otp.module, p, len + 1);
> +                       break;
> +               case 'V':
> +                       strscpy(devinfo->otp.vendor, p, len + 1);
> +                       break;
> +               case 'm':
> +                       strscpy(devinfo->otp.version, p, len + 1);
> +                       break;
> +               }
> +
> +               /* Skip to space separator or NUL */
> +               p = end;
> +       }
> +
> +       brcmf_dbg(PCIE, "OTP: module=%s vendor=%s version=%s\n",
> +                 devinfo->otp.module, devinfo->otp.vendor,
> +                 devinfo->otp.version);
> +
> +       if (!devinfo->otp.module ||
> +           !devinfo->otp.vendor ||
> +           !devinfo->otp.version)
> +               return -EINVAL;
> +
> +       devinfo->otp.valid = true;
> +       return 0;
> +}
> +
> +static int
> +brcmf_pcie_parse_otp(struct brcmf_pciedev_info *devinfo, u8 *otp, size_t size)
> +{
> +       int p = 0;

> +       int ret = -1;

Use proper error codes.

> +       brcmf_dbg(PCIE, "parse_otp size=%ld\n", size);
> +
> +       while (p < (size - 1)) {

too many parentheses

> +               u8 type = otp[p];
> +               u8 length = otp[p + 1];
> +
> +               if (type == 0)
> +                       break;
> +
> +               if ((p + 2 + length) > size)
> +                       break;
> +
> +               switch (type) {
> +               case BRCMF_OTP_SYS_VENDOR:
> +                       brcmf_dbg(PCIE, "OTP @ 0x%x (0x%x): SYS_VENDOR\n",

length as hex a bit harder to parse

> +                                 p, length);
> +                       ret = brcmf_pcie_parse_otp_sys_vendor(devinfo,
> +                                                             &otp[p + 2],
> +                                                             length);
> +                       break;
> +               case BRCMF_OTP_BRCM_CIS:
> +                       brcmf_dbg(PCIE, "OTP @ 0x%x (0x%x): BRCM_CIS\n",
> +                                 p, length);
> +                       break;
> +               default:
> +                       brcmf_dbg(PCIE, "OTP @ 0x%x (0x%x): Unknown type 0x%x\n",
> +                                 p, length, type);
> +                       break;
> +               }

> +               p += 2 + length;


length + 2 is easier to read.

> +       }
> +
> +       return ret;
> +}

...

> +               /* Map OTP to shadow area */
> +               WRITECC32(devinfo, sromcontrol,
> +                         sromctl | BCMA_CC_SROM_CONTROL_OTPSEL);

One line?

...

> +       otp = kzalloc(sizeof(u16) * words, GFP_KERNEL);

No check, why? I see in many places you forgot to check for NULL from
allocator functions.
Moreover here you should use kcalloc() which does overflow protection.

-- 
With Best Regards,
Andy Shevchenko

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ