| 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: <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