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?
> + 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
