[PATCH v3 0/3] nvmem: fix out-of-bounds reboot-mode write

[PATCH v3 0/3] nvmem: fix out-of-bounds reboot-mode write

[Jennifer Berringer]

Hi all,

These patches fix a small out-of-bounds write when using the
nvmem-reboot-mode driver on Qualcomm SA8775P, such as by executing
"reboot bootloader" in a shell. Relevant error log:

  BUG: KASAN: slab-out-of-bounds in nvmem_cell_prepare_write_buffer+0x144/0x47c
  Write of size 4 at addr ffff19dd8e1a37a0 by task systemd-shutdow/1
  Hardware name: Qualcomm SA8775P Ride (DT)
  Call trace:
  nvmem_cell_prepare_write_buffer
  nvmem_cell_write
  nvmem_reboot_mode_write
  The buggy address is located 0 bytes inside of
   allocated 1-byte region

This problem manifested because the devicetree file sa8775p-pmics.dtsi
specifies its reboot-mode cell with "reg = <0x48 0x1>" and so expects
the reboot mode write to only be 1 byte rather than 4. Other in-tree
devicetrees that seem likely affected include pmk8350 and pmk8550.

These changes add the missing bounds check to nvmem_cell_write and make
nvmem-reboot-mode able to only write as many bytes as it needs to.

v2:
- Added missing function definition for CONFIG_NVMEM=n

v3:
- Accepted suggestion for changing nvmem_cell_write bounds check

---
Jennifer Berringer (3):
  nvmem: core: improve range check for nvmem_cell_write()
  nvmem: core: add nvmem_cell_write_variable_u32()
  power: reset: nvmem-reboot-mode: fix write for small cells

 drivers/nvmem/core.c                    | 26 +++++++++++++++++++++++++
 drivers/power/reset/nvmem-reboot-mode.c |  2 +-
 include/linux/nvmem-consumer.h          |  6 ++++++
 3 files changed, 33 insertions(+), 1 deletion(-)


base-commit: 98f7e32f20d28ec452afb208f9cffc08448a2652
-- 
2.46.2

[PATCH v3 1/3] nvmem: core: improve range check for nvmem_cell_write()

[Jennifer Berringer]

When __nvmem_cell_entry_write() is called for an nvmem cell that does
not need bit shifting, it requires that the len parameter exactly
matches the nvmem cell size. However, when the nvmem cell has a nonzero
bit_offset, it was skipping this check.

Accepting values of len larger than the cell size results in
nvmem_cell_prepare_write_buffer() trying to write past the end of a heap
buffer that it allocates. Add a check to avoid that problem and instead
return -EINVAL when len doesn't match the number of bits expected by the
nvmem cell when bit_offset is nonzero.

This check uses cell->nbits in order to allow providing the smaller size
to cells that are shifted into another byte by bit_offset. For example,
a cell with nbits=8 and nonzero bit_offset would have bytes=2 but should
accept a 1-byte write here, although no current callers depend on this.

Fixes: 69aba7948cbe ("nvmem: Add a simple NVMEM framework for consumers")

Signed-off-by: Jennifer Berringer <jberring@redhat.com>
---
 drivers/nvmem/core.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index 33ffa2aa4c11..4a5a6efe4bab 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -1780,6 +1780,8 @@ static int __nvmem_cell_entry_write(struct nvmem_cell_entry *cell, void *buf, si
 		return -EINVAL;
 
 	if (cell->bit_offset || cell->nbits) {
+		if (len != BITS_TO_BYTES(cell->nbits) && len != cell->bytes)
+			return -EINVAL;
 		buf = nvmem_cell_prepare_write_buffer(cell, buf, len);
 		if (IS_ERR(buf))
 			return PTR_ERR(buf);
-- 
2.46.2

[PATCH v3 2/3] nvmem: core: add nvmem_cell_write_variable_u32()

[Jennifer Berringer]

This function allows nvmem consumers to write values of different sizes
(1-4 bytes) to an nvmem cell without knowing the exact size, akin to a
write counterpart to nvmem_cell_read_variable_le_32(). It discards the
higher order bytes of the passed u32 value based on CPU endianness as
necessary before writing to a cell smaller than 4 bytes.

Signed-off-by: Jennifer Berringer <jberring@redhat.com>
---
 drivers/nvmem/core.c           | 24 ++++++++++++++++++++++++
 include/linux/nvmem-consumer.h |  6 ++++++
 2 files changed, 30 insertions(+)

diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index 4a5a6efe4bab..4c26a5e8c361 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -1815,6 +1815,30 @@ int nvmem_cell_write(struct nvmem_cell *cell, void *buf, size_t len)
 
 EXPORT_SYMBOL_GPL(nvmem_cell_write);
 
+/**
+ * nvmem_cell_write_variable_u32() - Write up to 32-bits of data as a host-endian number
+ *
+ * @cell: nvmem cell to be written.
+ * @val: Value to be written which may be truncated.
+ *
+ * Return: length of bytes written or negative on failure.
+ */
+int nvmem_cell_write_variable_u32(struct nvmem_cell *cell, u32 val)
+{
+	struct nvmem_cell_entry *entry = cell->entry;
+	u8 *buf = (u8 *) &val;
+
+	if (!entry || entry->bytes > sizeof(u32))
+		return -EINVAL;
+
+#ifdef __BIG_ENDIAN
+	buf += sizeof(u32) - entry->bytes;
+#endif
+
+	return __nvmem_cell_entry_write(entry, buf, entry->bytes);
+}
+EXPORT_SYMBOL_GPL(nvmem_cell_write_variable_u32);
+
 static int nvmem_cell_read_common(struct device *dev, const char *cell_id,
 				  void *val, size_t count)
 {
diff --git a/include/linux/nvmem-consumer.h b/include/linux/nvmem-consumer.h
index 34c0e58dfa26..955366a07867 100644
--- a/include/linux/nvmem-consumer.h
+++ b/include/linux/nvmem-consumer.h
@@ -56,6 +56,7 @@ void nvmem_cell_put(struct nvmem_cell *cell);
 void devm_nvmem_cell_put(struct device *dev, struct nvmem_cell *cell);
 void *nvmem_cell_read(struct nvmem_cell *cell, size_t *len);
 int nvmem_cell_write(struct nvmem_cell *cell, void *buf, size_t len);
+int nvmem_cell_write_variable_u32(struct nvmem_cell *cell, u32 val);
 int nvmem_cell_read_u8(struct device *dev, const char *cell_id, u8 *val);
 int nvmem_cell_read_u16(struct device *dev, const char *cell_id, u16 *val);
 int nvmem_cell_read_u32(struct device *dev, const char *cell_id, u32 *val);
@@ -128,6 +129,11 @@ static inline int nvmem_cell_write(struct nvmem_cell *cell,
 	return -EOPNOTSUPP;
 }
 
+static inline int nvmem_cell_write_variable_u32(struct nvmem_cell *cell, u32 val)
+{
+	return -EOPNOTSUPP;
+}
+
 static inline int nvmem_cell_read_u8(struct device *dev,
 				     const char *cell_id, u8 *val)
 {
-- 
2.46.2

[PATCH v3 3/3] power: reset: nvmem-reboot-mode: fix write for small cells

[Jennifer Berringer]

Some devices, such as Qualcomm sa8775p, have an nvmem reboot mode cell
that is smaller than 32 bits, which resulted in
nvmem_reboot_mode_write() failing. Using nvmem_cell_write_variable_u32()
fixes this by writing only the least-significant byte of the magic value
when the size specified in device tree is only one byte.

Signed-off-by: Jennifer Berringer <jberring@redhat.com>
---
 drivers/power/reset/nvmem-reboot-mode.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/power/reset/nvmem-reboot-mode.c b/drivers/power/reset/nvmem-reboot-mode.c
index 41530b70cfc4..b52eb879d1c1 100644
--- a/drivers/power/reset/nvmem-reboot-mode.c
+++ b/drivers/power/reset/nvmem-reboot-mode.c
@@ -24,7 +24,7 @@ static int nvmem_reboot_mode_write(struct reboot_mode_driver *reboot,
 
 	nvmem_rbm = container_of(reboot, struct nvmem_reboot_mode, reboot);
 
-	ret = nvmem_cell_write(nvmem_rbm->cell, &magic, sizeof(magic));
+	ret = nvmem_cell_write_variable_u32(nvmem_rbm->cell, magic);
 	if (ret < 0)
 		dev_err(reboot->dev, "update reboot mode bits failed\n");
 
-- 
2.46.2

Re: [PATCH v3 3/3] power: reset: nvmem-reboot-mode: fix write for small cells

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 1334 bytes --]

Hi,

On Mon, Nov 04, 2024 at 10:23:12AM -0500, Jennifer Berringer wrote:
> Some devices, such as Qualcomm sa8775p, have an nvmem reboot mode cell
> that is smaller than 32 bits, which resulted in
> nvmem_reboot_mode_write() failing. Using nvmem_cell_write_variable_u32()
> fixes this by writing only the least-significant byte of the magic value
> when the size specified in device tree is only one byte.
> 
> Signed-off-by: Jennifer Berringer <jberring@redhat.com>
> ---

Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.com>

-- Sebastian

>  drivers/power/reset/nvmem-reboot-mode.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/power/reset/nvmem-reboot-mode.c b/drivers/power/reset/nvmem-reboot-mode.c
> index 41530b70cfc4..b52eb879d1c1 100644
> --- a/drivers/power/reset/nvmem-reboot-mode.c
> +++ b/drivers/power/reset/nvmem-reboot-mode.c
> @@ -24,7 +24,7 @@ static int nvmem_reboot_mode_write(struct reboot_mode_driver *reboot,
>  
>  	nvmem_rbm = container_of(reboot, struct nvmem_reboot_mode, reboot);
>  
> -	ret = nvmem_cell_write(nvmem_rbm->cell, &magic, sizeof(magic));
> +	ret = nvmem_cell_write_variable_u32(nvmem_rbm->cell, magic);
>  	if (ret < 0)
>  		dev_err(reboot->dev, "update reboot mode bits failed\n");
>  
> -- 
> 2.46.2
> 
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v3 2/3] nvmem: core: add nvmem_cell_write_variable_u32()

[Srinivas Kandagatla]

Thanks for the patch,

On 04/11/2024 15:23, Jennifer Berringer wrote:
> This function allows nvmem consumers to write values of different sizes
> (1-4 bytes) to an nvmem cell without knowing the exact size, akin to a
> write counterpart to nvmem_cell_read_variable_le_32(). It discards the
> higher order bytes of the passed u32 value based on CPU endianness as
> necessary before writing to a cell smaller than 4 bytes.
> 
> Signed-off-by: Jennifer Berringer <jberring@redhat.com>
> ---
>   drivers/nvmem/core.c           | 24 ++++++++++++++++++++++++
>   include/linux/nvmem-consumer.h |  6 ++++++
>   2 files changed, 30 insertions(+)
> 
> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
> index 4a5a6efe4bab..4c26a5e8c361 100644
> --- a/drivers/nvmem/core.c
> +++ b/drivers/nvmem/core.c
> @@ -1815,6 +1815,30 @@ int nvmem_cell_write(struct nvmem_cell *cell, void *buf, size_t len)
>   
>   EXPORT_SYMBOL_GPL(nvmem_cell_write);
>   
> +/**
> + * nvmem_cell_write_variable_u32() - Write up to 32-bits of data as a host-endian number

> + *
> + * @cell: nvmem cell to be written.
> + * @val: Value to be written which may be truncated.
> + *
> + * Return: length of bytes written or negative on failure.
> + */
> +int nvmem_cell_write_variable_u32(struct nvmem_cell *cell, u32 val)

This new API is confusing, as writing to cell should in the same order 
of the data. Doing any data manipulation within the api is confusing to 
users.

If the intention is to know the size of cell before writing, then best 
way to address this is to provide the cell size visibility to the consumer.

--srini

> +{
> +	struct nvmem_cell_entry *entry = cell->entry;
> +	u8 *buf = (u8 *) &val;
> +
> +	if (!entry || entry->bytes > sizeof(u32))
> +		return -EINVAL;
> +

> +#ifdef __BIG_ENDIAN
> +	buf += sizeof(u32) - entry->bytes;
> +#endif
> +
> +	return __nvmem_cell_entry_write(entry, buf, entry->bytes);
> +}
> +EXPORT_SYMBOL_GPL(nvmem_cell_write_variable_u32);
> +
>   static int nvmem_cell_read_common(struct device *dev, const char *cell_id,
>   				  void *val, size_t count)
>   {
> diff --git a/include/linux/nvmem-consumer.h b/include/linux/nvmem-consumer.h
> index 34c0e58dfa26..955366a07867 100644
> --- a/include/linux/nvmem-consumer.h
> +++ b/include/linux/nvmem-consumer.h
> @@ -56,6 +56,7 @@ void nvmem_cell_put(struct nvmem_cell *cell);
>   void devm_nvmem_cell_put(struct device *dev, struct nvmem_cell *cell);
>   void *nvmem_cell_read(struct nvmem_cell *cell, size_t *len);
>   int nvmem_cell_write(struct nvmem_cell *cell, void *buf, size_t len);
> +int nvmem_cell_write_variable_u32(struct nvmem_cell *cell, u32 val);
>   int nvmem_cell_read_u8(struct device *dev, const char *cell_id, u8 *val);
>   int nvmem_cell_read_u16(struct device *dev, const char *cell_id, u16 *val);
>   int nvmem_cell_read_u32(struct device *dev, const char *cell_id, u32 *val);
> @@ -128,6 +129,11 @@ static inline int nvmem_cell_write(struct nvmem_cell *cell,
>   	return -EOPNOTSUPP;
>   }
>   
> +static inline int nvmem_cell_write_variable_u32(struct nvmem_cell *cell, u32 val)
> +{
> +	return -EOPNOTSUPP;
> +}
> +
>   static inline int nvmem_cell_read_u8(struct device *dev,
>   				     const char *cell_id, u8 *val)
>   {

Re: (subset) [PATCH v3 0/3] nvmem: fix out-of-bounds reboot-mode write

[Srinivas Kandagatla]

On Mon, 04 Nov 2024 10:23:09 -0500, Jennifer Berringer wrote:
> These patches fix a small out-of-bounds write when using the
> nvmem-reboot-mode driver on Qualcomm SA8775P, such as by executing
> "reboot bootloader" in a shell. Relevant error log:
> 
>   BUG: KASAN: slab-out-of-bounds in nvmem_cell_prepare_write_buffer+0x144/0x47c
>   Write of size 4 at addr ffff19dd8e1a37a0 by task systemd-shutdow/1
>   Hardware name: Qualcomm SA8775P Ride (DT)
>   Call trace:
>   nvmem_cell_prepare_write_buffer
>   nvmem_cell_write
>   nvmem_reboot_mode_write
>   The buggy address is located 0 bytes inside of
>    allocated 1-byte region
> 
> [...]

Applied, thanks!

[1/3] nvmem: core: improve range check for nvmem_cell_write()
      commit: 27a3485271ed3e81dec1e51460dc671dae387a2a

Best regards,
-- 
Srinivas Kandagatla <srinivas.kandagatla@linaro.org>

Re: [PATCH v3 2/3] nvmem: core: add nvmem_cell_write_variable_u32()

[Jennifer Berringer]

Thanks for the feedback and for accepting my first patch.

On 12/14/24 10:07, Srinivas Kandagatla wrote:
>> + * nvmem_cell_write_variable_u32() - Write up to 32-bits of data as a host-endian number
> 
>> + *
>> + * @cell: nvmem cell to be written.
>> + * @val: Value to be written which may be truncated.
>> + *
>> + * Return: length of bytes written or negative on failure.
>> + */
>> +int nvmem_cell_write_variable_u32(struct nvmem_cell *cell, u32 val)
> 
> This new API is confusing, as writing to cell should in the same order of the data. Doing any data manipulation within the api is confusing to users.
> 
> If the intention is to know the size of cell before writing, then best way to address this is to provide the cell size visibility to the consumer.
> 
> --srini

My intention was to mirror the existing functions in this file, hoping that would make it less confusing rather than more. nvmem_cell_read_variable_le_u32() already similarly has consumers accessing the contents of a cell without knowing its size, silently filling any bytes not read with zero. The function I add doesn't change the order of the bytes. The existing read_variable_le functions (in contrast) byte swap from little-endian to the CPU's endianness and indicate this in the function name. Otherwise, I believe the function I add here is what would be expected from a write equivalent. Its return value also gives the caller the actual cell size upon success.

The only manipulation done to the data here is truncating to ignore the highest-order bytes. This behavior should match the below example unless the size is 3 bytes, which my patch should handle but the below example can't.

if (entry->bytes == sizeof(u32)) {
    return __nvmem_cell_entry_write(entry, &val, sizeof(u32));
} else if (entry->bytes == sizeof(u16)) {
    u16 val_16 = (u16) val;
    return __nvmem_cell_entry_write(entry, &val_16, sizeof(u16));
} else if (entry->bytes == sizeof(u8)) {
    u8 val_8 = (u8) val;
    return __nvmem_cell_entry_write(entry, &val_8, sizeof(u8));
} else {
    return -EINVAL;
}

Still, if you prefer, I can send new patches that add a function to get the cell size and put any truncation behavior needed by nvmem-reboot-mode into that source file instead.

Re: [PATCH v3 2/3] nvmem: core: add nvmem_cell_write_variable_u32()

[Srinivas Kandagatla]

On 20/12/2024 19:39, Jennifer Berringer wrote:
> Thanks for the feedback and for accepting my first patch.
> 
> On 12/14/24 10:07, Srinivas Kandagatla wrote:
>>> + * nvmem_cell_write_variable_u32() - Write up to 32-bits of data as a host-endian number
>>
>>> + *
>>> + * @cell: nvmem cell to be written.
>>> + * @val: Value to be written which may be truncated.
>>> + *
>>> + * Return: length of bytes written or negative on failure.
>>> + */
>>> +int nvmem_cell_write_variable_u32(struct nvmem_cell *cell, u32 val)
>>
>> This new API is confusing, as writing to cell should in the same order of the data. Doing any data manipulation within the api is confusing to users.
>>
>> If the intention is to know the size of cell before writing, then best way to address this is to provide the cell size visibility to the consumer.
>>
>> --srini
> 
> My intention was to mirror the existing functions in this file, hoping that would make it less confusing rather than more. nvmem_cell_read_variable_le_u32() already similarly has consumers accessing the contents of a cell without knowing its size, silently filling any bytes not read with zero. The function I add doesn't change the order of the bytes. The existing read_variable_le functions (in contrast) byte swap from little-endian to the CPU's endianness and indicate this in the function name. Otherwise, I believe the function I add here is what would be expected from a write equivalent. Its return value also gives the caller the actual cell size upon success.
> 

Writes happen in the order of data that is provided, and read in the 
same order that is stored in NVMEM.  Reading from nvmem storage can be 
translated into whatever order that consumer wants which is what the 
read helpers are doing. On the other hand writes should be written in 
the same order as its received.

Note that the order in which EEPROM stores data might not be the same 
order as CPU.

Adding Endianess in writing to a common code is a problem. If you are 
certain that the NVMEM needs particular order(le/be) then either convert 
(cpu_to_le/cpu_to_be) the data before it hits write call.



> The only manipulation done to the data here is truncating to ignore the highest-order bytes. This behavior should match the below example unless the size is 3 bytes, which my patch should handle but the below example can't.
> 
> if (entry->bytes == sizeof(u32)) {
>      return __nvmem_cell_entry_write(entry, &val, sizeof(u32));
> } else if (entry->bytes == sizeof(u16)) {
>      u16 val_16 = (u16) val;
>      return __nvmem_cell_entry_write(entry, &val_16, sizeof(u16));
> } else if (entry->bytes == sizeof(u8)) {
>      u8 val_8 = (u8) val;
>      return __nvmem_cell_entry_write(entry, &val_8, sizeof(u8));
> } else {
>      return -EINVAL;
> }
> 
> Still, if you prefer, I can send new patches that add a function to get the cell size and put any truncation behavior needed by nvmem-reboot-mode into that source file instead.

there are two things, one is the variable size u32, other is changing 
the endainess. Am happy with the former one as long as the consumer is 
taking care of order.

But the later one is No, as explained.

--srini
>