| 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: <4625b434-ed73-d266-0340-76c8bd8fd46e@marcan.st>
Date: Fri, 7 Jan 2022 12:53:24 +0900
From: Hector Martin <marcan@...can.st>
To: Andy Shevchenko <andy.shevchenko@...il.com>
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 2022/01/04 20:26, Andy Shevchenko wrote:
> 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?
That would mean I need to copy, since the original data is not aligned.
Since it's just one u32 header (which we don't even truly know the
interpretation of), it seems get_unaligned_le32 is easier.
>
>> + 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() ?
We're splitting on \0 here, so that won't work.
>
>> + 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()
Sure.
>
>> +
>> + if (*p++ != '=') /* implicit NUL check */
>> + return -EINVAL;
>
> Have you checked the next_arg() implementation?
That function has a lot more logic (handling quotes, etc) and no other
hardware drivers use it. I'm not sure I feel comfortable using it to
parse untrusted data from a potentially compromised device. The parsing
we need to do here is much simpler.
>
>> + /* *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.
Ack.
>
>> + brcmf_dbg(PCIE, "parse_otp size=%ld\n", size);
>> +
>> + while (p < (size - 1)) {
>
> too many parentheses
Really? I see this is all over kernel code. I know it's redundant, but I
find parentheses around expressions used for one side of a comparison to
be a lot more readable since you don't have to start doubting whether
that particular operator has higher precedence than the comparison (+
does but & does not).
>
>> + 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
Not so sure about that, especially if you're trying to mentally add it
to offsets... but sure, I can make it decimal.
>
>> + 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.
I was following the data order here; 2 header bytes and then length
payload bytes. Same reason I used p + 2 + length above.
>
>> + }
>> +
>> + return ret;
>> +}
>
> ...
>
>> + /* Map OTP to shadow area */
>> + WRITECC32(devinfo, sromcontrol,
>> + sromctl | BCMA_CC_SROM_CONTROL_OTPSEL);
>
> One line?
That exceeds 80 chars, which seems to be the standard in this file which
I'm trying to stick to. If people are okay with pushing to 100 lines,
there are lots of other places I could unwrap lines in this series.
>
> ...
>
>> + otp = kzalloc(sizeof(u16) * words, GFP_KERNEL);
>
> No check, why? I see in many places you forgot to check for NULL from
> allocator functions.
I think at some point something convinced me that kzalloc and friends
don't fail with GFP_KERNEL... which they rarely do, but they do. I'll
fix it, and add a few missing checks to the existing code while I'm at it.
> Moreover here you should use kzalloc() which does overflow protection.
words is a constant from the switch statement so this could never
overflow anyway, but sure.
--
Hector Martin (marcan@...can.st)
Public Key: https://mrcn.st/pub
Powered by blists - more mailing lists