[PATCH v2] mtd: cfi_cmdset_0001: Byte swap OTP info

[Nicolas Pitre]

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') {
>