Re: [PATCH v3 2/4] nvmem: make all reg_write callbacks take const void *

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:

New issues:
- [Medium] Incomplete API migration leaving deprecated reg_write callbacks active

Pre-existing issues:
- [High] kCFI panic due to function pointer cast of regmap_bulk_write
- [High] Hardware state corruption and out-of-bounds read in sprd_efuse_write
- [High] OP-TEE session resource leak on alignment check failure
- [High] Missing locking leads to state corruption across multiple backend drivers
- [High] Sleeping in atomic context within sprd-efuse driver
- [High] Security check bypass during multi-word writes in zynqmp_nvmem_write
--

--- Patch [2]: [PATCH v3 2/4] nvmem: make all reg_write callbacks take const void * ---
commit ec203eda05f126cbe777e1158095fb9e39499796
Author: Link Mauve <linkmauve@linkmauve.fr>

nvmem: make all reg_write callbacks take const void *

This patch updates various nvmem drivers to use the new reg_write_const
callback instead of reg_write, matching a recent API change that passes
immutable data.

> The previous commit switched from a pointer to mutable data to a pointer
> to immutable data, so let’s fix all users of this API.

[Severity: Medium]
Are there some drivers missing from this update? It appears several nvmem
backend drivers are still assigning the deprecated reg_write callback,
which leaves the migration incomplete:

drivers/nvmem/at24.c:at24_probe() {
    nvmem_config.reg_write = at24_write;
}

drivers/nvmem/at25.c:at25_probe() {
    at25->nvmem_config.reg_write = at25_ee_write;
}

drivers/nvmem/eeprom_93xx46.c:eeprom_93xx46_probe() {
    edev->nvmem_config.reg_write = eeprom_93xx46_write;
}

drivers/nvmem/m24lr.c:m24lr_probe() {
    nvmem_conf.reg_write = m24lr_nvmem_write;
}

> diff --git a/drivers/nvmem/apple-spmi-nvmem.c b/drivers/nvmem/apple-spmi-nvmem.c
> index 88614005d5ce1..cbf25c53d048d 100644
> --- a/drivers/nvmem/apple-spmi-nvmem.c
> +++ b/drivers/nvmem/apple-spmi-nvmem.c

[ ... ]

> @@ -29,7 +29,7 @@ static int apple_spmi_nvmem_probe(struct spmi_device *sdev)
>  		.stride = 1,
>  		.size = 0xffff,
>  		.reg_read = (void *)regmap_bulk_read,
> -		.reg_write = (void *)regmap_bulk_write,
> +		.reg_write_const = (void *)regmap_bulk_write,

[Severity: High]
This is a pre-existing issue, but does casting regmap_bulk_write to void *
trigger a kCFI kernel panic? The actual signature of regmap_bulk_write
doesn't match reg_write_const, so calling it via this function pointer
could cause a control-flow integrity trap upon execution.

>  	};

[ ... ]

> diff --git a/drivers/nvmem/bcm-ocotp.c b/drivers/nvmem/bcm-ocotp.c
> index 2490f44caa409..02adfd787de85 100644
> --- a/drivers/nvmem/bcm-ocotp.c
> +++ b/drivers/nvmem/bcm-ocotp.c

[ ... ]

> @@ -179,11 +179,11 @@ static int bcm_otpc_read(void *context, unsigned int offset, void *val,
>  	return 0;
>  }
>  
> -static int bcm_otpc_write(void *context, unsigned int offset, void *val,
> -	size_t bytes)
> +static int bcm_otpc_write(void *context, unsigned int offset, const void *val,
> +			  size_t bytes)
>  {

[Severity: High]
This is a pre-existing issue, but does this function lack locking?
Modifying shared command and address registers concurrently without a lock
could corrupt the shared state machine:

drivers/nvmem/bcm-ocotp.c:bcm_otpc_write() {
    for (bytes_written = 0; bytes_written < bytes;) {
        set_command(priv->base, OTPC_CMD_PROGRAM);
        set_cpu_address(priv->base, address++);
        ...
}

>  	struct otpc_priv *priv = context;

[ ... ]

> diff --git a/drivers/nvmem/lan9662-otpc.c b/drivers/nvmem/lan9662-otpc.c
> index 62d1d6381bf88..f586ae4cd126b 100644
> --- a/drivers/nvmem/lan9662-otpc.c
> +++ b/drivers/nvmem/lan9662-otpc.c

[ ... ]

> @@ -139,10 +139,10 @@ static int lan9662_otp_read(void *context, unsigned int offset,
>  }
>  
>  static int lan9662_otp_write(void *context, unsigned int offset,
> -			     void *_val, size_t bytes)
> +			     const void *_val, size_t bytes)
>  {

[Severity: High]
This is a pre-existing issue, but does this function perform unprotected
read-modify-write operations on OTP bytes? Modifying the OTP bytes without
locking could lead to data corruption in concurrent executions:

drivers/nvmem/lan9662-otpc.c:lan9662_otp_write() {
    for (i = 0; i < bytes; i++) {
        if (val[i]) {
            rc = lan9662_otp_read_byte(otp, offset + i, &data);
            ...
            rc = lan9662_otp_write_byte(otp, offset + i, newdata);
        }
    }
}

>  	struct lan9662_otp *otp = context;

[ ... ]

> diff --git a/drivers/nvmem/lpc18xx_eeprom.c b/drivers/nvmem/lpc18xx_eeprom.c
> index 504155e30bab2..78a35ef095fbd 100644
> --- a/drivers/nvmem/lpc18xx_eeprom.c
> +++ b/drivers/nvmem/lpc18xx_eeprom.c

[ ... ]

> @@ -87,7 +87,7 @@ static int lpc18xx_eeprom_busywait_until_prog(struct lpc18xx_eeprom_dev *eeprom)
>  }
>  
>  static int lpc18xx_eeprom_gather_write(void *context, unsigned int reg,
> -				       void *val, size_t bytes)
> +				       const void *val, size_t bytes)
>  {

[Severity: High]
This isn't a bug introduced by this patch, but does this function lack
locking when modifying shared power and control registers? Concurrent
accesses could corrupt the shared state:

drivers/nvmem/lpc18xx_eeprom.c:lpc18xx_eeprom_gather_write() {
    ...
    lpc18xx_eeprom_writel(eeprom, LPC18XX_EEPROM_PWRDWN,
                          LPC18XX_EEPROM_PWRDWN_NO);
    ...
}

>  	struct lpc18xx_eeprom_dev *eeprom = context;

[ ... ]

> diff --git a/drivers/nvmem/qfprom.c b/drivers/nvmem/qfprom.c
> index 1de3435df116d..16b7f647a3f6f 100644
> --- a/drivers/nvmem/qfprom.c
> +++ b/drivers/nvmem/qfprom.c

[ ... ]

> @@ -252,13 +252,13 @@ static int qfprom_enable_fuse_blowing(const struct qfprom_priv *priv,
>   *
>   * Return: 0 or -err.
>   */
> -static int qfprom_reg_write(void *context, unsigned int reg, void *_val,
> +static int qfprom_reg_write(void *context, unsigned int reg, const void *_val,
>  			    size_t bytes)
>  {

[Severity: High]
This is a pre-existing issue, but does this function need synchronization?
Modifying shared clocks and voltage states without locking could cause
state corruption:

drivers/nvmem/qfprom.c:qfprom_reg_write() {
    ...
    ret = qfprom_enable_fuse_blowing(priv, &old);
    ...
}

>  	struct qfprom_priv *priv = context;

[ ... ]

> diff --git a/drivers/nvmem/sprd-efuse.c b/drivers/nvmem/sprd-efuse.c
> index 1a7e4e5d8b86c..f960fa8cb25cf 100644
> --- a/drivers/nvmem/sprd-efuse.c
> +++ b/drivers/nvmem/sprd-efuse.c

[ ... ]

> @@ -192,7 +192,7 @@ static void sprd_efuse_set_prog_en(struct sprd_efuse *efuse, bool en)
>  }
>  
>  static int sprd_efuse_raw_prog(struct sprd_efuse *efuse, u32 blk, bool doub,
> -			       bool lock, u32 *data)
> +			       bool lock, const u32 *data)
>  {

[Severity: High]
This isn't a bug introduced by this patch, but does this unconditionally
dereference the buffer as a u32 pointer? If the payload is smaller than 4
bytes, this could result in an out-of-bounds read:

drivers/nvmem/sprd-efuse.c:sprd_efuse_raw_prog() {
    ...
    writel(*data, efuse->base + SPRD_EFUSE_MEM(blk));
    ...
}

>  	u32 status;
>  	int ret = 0;

[ ... ]

> @@ -321,7 +321,7 @@ static int sprd_efuse_read(void *context, u32 offset, void *val, size_t bytes)
>  	return ret;
>  }
>  
> -static int sprd_efuse_write(void *context, u32 offset, void *val, size_t bytes)
> +static int sprd_efuse_write(void *context, u32 offset, const void *val, size_t bytes)
>  {

[Severity: High]
This is a pre-existing issue, but can this function sleep in atomic context?
If sprd_efuse_lock() acquires a hardware spinlock, calling the sleepable
clk_prepare_enable() could trigger a locking violation and soft lockup.
This seems to affect both sprd_efuse_write() and sprd_efuse_read():

drivers/nvmem/sprd-efuse.c:sprd_efuse_write() {
    ret = sprd_efuse_lock(efuse);
    if (ret)
        return ret;
    
    ret = clk_prepare_enable(efuse->clk);
    ...
}

Additionally, is the raw byte offset passed directly as a block index here?
Targeting incorrect addresses could lead to hardware state corruption:

drivers/nvmem/sprd-efuse.c:sprd_efuse_write() {
    ...
    ret = sprd_efuse_raw_prog(efuse, offset, blk_double, lock, val);
    ...
}

>  	struct sprd_efuse *efuse = context;

[ ... ]

> diff --git a/drivers/nvmem/stm32-bsec-optee-ta.c b/drivers/nvmem/stm32-bsec-optee-ta.c
> index f89ce791dd12a..3184b976b1f91 100644
> --- a/drivers/nvmem/stm32-bsec-optee-ta.c
> +++ b/drivers/nvmem/stm32-bsec-optee-ta.c

[ ... ]

> @@ -203,7 +203,7 @@ int stm32_bsec_optee_ta_read(struct tee_context *ctx, unsigned int offset,
>  
>  /* stm32_bsec_optee_ta_write() - nvmem write access using PTA client driver */
>  int stm32_bsec_optee_ta_write(struct tee_context *ctx, unsigned int lower,
> -			      unsigned int offset, void *buf, size_t bytes)
> +			      unsigned int offset, const void *buf, size_t bytes)
>  {	struct tee_shm *shm;

[Severity: High]
This is a pre-existing issue, but does this function leak the OP-TEE
session if the alignment check fails? Returning directly bypasses cleanup:

drivers/nvmem/stm32-bsec-optee-ta.c:stm32_bsec_optee_ta_write() {
    ret = stm32_bsec_ta_open_session(ctx, &session_id);
    if (ret)
        return ret;

    /* Allow only writing complete 32-bits aligned words */
    if ((bytes % 4) || (offset % 4))
        return -EINVAL;
    ...
}

[ ... ]

> diff --git a/drivers/nvmem/zynqmp_nvmem.c b/drivers/nvmem/zynqmp_nvmem.c
> index d297ff150dc02..54905a608157d 100644
> --- a/drivers/nvmem/zynqmp_nvmem.c
> +++ b/drivers/nvmem/zynqmp_nvmem.c

[ ... ]

> @@ -183,7 +183,7 @@ static int zynqmp_nvmem_read(void *context, unsigned int offset, void *val, size
>  }
>  
>  static int zynqmp_nvmem_write(void *context,
> -			      unsigned int offset, void *val, size_t bytes)
> +			      unsigned int offset, const void *val, size_t bytes)
>  {

[Severity: High]
This isn't a bug introduced by this patch, but does zynqmp_efuse_access()
bypass local bitmask validations for subsequent words during a multi-word
write? The check seems to only happen on the first word:

drivers/nvmem/zynqmp_nvmem.c:zynqmp_efuse_access() {
    if (pufflag == 1 && flag == EFUSE_WRITE) {
        memcpy(&value, val, sizeof(value));
        if ((offset == EFUSE_PUF_START_OFFSET ||
             offset == EFUSE_PUF_MID_OFFSET) &&
            value & P_USER_0_64_UPPER_MASK) {
            ...
        }
    ...
}

>  	int pufflag = 0;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260716142415.28718-1-linkmauve@linkmauve.fr?part=2

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic