On Thu, 1 Jun 2023, Nicolas Pitre wrote:
> On Thu, 1 Jun 2023, Linus Walleij wrote:
>
> > Currently the offset into the device when looking for OTP
> > bits can go outside of the address of the MTD NOR devices,
> > and if that memory isn't readable, bad things happen
> > on the IXP4xx (added prints that illustrate the problem before
> > the crash):
> >
> > cfi_intelext_otp_walk walk OTP on chip 0 start at reg_prot_offset 0x00000100
> > ixp4xx_copy_from copy from 0x00000100 to 0xc880dd78
> > cfi_intelext_otp_walk walk OTP on chip 0 start at reg_prot_offset 0x12000000
> > ixp4xx_copy_from copy from 0x12000000 to 0xc880dd78
> > 8<--- cut here ---
> > Unable to handle kernel paging request at virtual address db000000
> > [db000000] *pgd=00000000
> > (...)
> >
> > This happens in this case because the IXP4xx is big endian and
> > the 32- and 16-bit fields in the struct cfi_intelext_otpinfo are not
> > properly byteswapped. Compare to how the code in read_pri_intelext()
> > byteswaps the fields in struct cfi_pri_intelext.
>
> DOH!
>
> And to confirm, in include/linux/mtd/cfi.h we can see:
>
> /* NB: We keep these structures in memory in HOST byteorder, except
> * where individually noted.
> */
>
> > Adding some byte swapping after casting the &extp->extra[0] into
> > a struct cfi_intelext_otpinfo * pointer, and the crash goes away.
>
> But this is wrong to do so in cfi_intelext_otp_walk(). That function is
> a helper applying given operation callback on given range. Each time it
> is called those values will be swapped back and forth. You want to do
> the swap only once during init in read_pri_intelext().
>
> Something like this (completely untested):
>
> diff --git a/drivers/mtd/chips/cfi_cmdset_0001.c b/drivers/mtd/chips/cfi_cmdset_0001.c
> index 54f92d09d9..723dd6473c 100644
> --- a/drivers/mtd/chips/cfi_cmdset_0001.c
> +++ b/drivers/mtd/chips/cfi_cmdset_0001.c
> @@ -421,9 +421,20 @@ read_pri_intelext(struct map_info *map, __u16 adr)
> extra_size = 0;
>
> /* Protection Register info */
> - if (extp->NumProtectionFields)
> + if (extp->NumProtectionFields) {
> + struct cfi_intelext_otpinfo *otp =
> + (struct cfi_intelext_otpinfo *)&extp->extra[0];
> +
> extra_size += (extp->NumProtectionFields - 1) *
> sizeof(struct cfi_intelext_otpinfo);
> + if (extp_size >= sizeof(*extp) + extra_size) {
> + for (i = 0; i < extp->NumProtectionFields - 1; i++) {
> + otp->ProtRegAddr = le32_to_cpu(otp->ProtRegAddr);
> + otp->FactGroups = le16_to_cpu(otp->FactGroups);
> + otp->UserGroups = le16_to_cpu(otp->UserGroups);
---> plus the missing otp++ here;
> + }
> + }
> + }
> }
>
> if (extp->MinorVersion >= '1') {
>
