On 2022/01/04 20:26, Andy Shevchenko wrote:
> On Tue, Jan 4, 2022 at 9:28 AM Hector Martin <marcan@xxxxxxxxx> 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@xxxxxxxxx)
Public Key: https://mrcn.st/pub
