[PATCH v2 0/3] nvmem: fix out-of-bounds write

[PATCH v2 0/3] nvmem: fix out-of-bounds 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

---
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                    | 31 +++++++++++++++++++++++--
 drivers/power/reset/nvmem-reboot-mode.c |  2 +-
 include/linux/nvmem-consumer.h          |  6 +++++
 3 files changed, 36 insertions(+), 3 deletions(-)


base-commit: 98f7e32f20d28ec452afb208f9cffc08448a2652
-- 
2.46.2

[PATCH v2 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. This patch adds a check to avoid that problem
and instead return -EINVAL when len is too large.

Rather than unconditionally checking that len exactly matches the nvmem
cell size, allowing len to be smaller when bit shifts are involved may
be helpful because some devices have nvmem cells that are less than 8
bits but span two bytes, although no current devices or drivers that do
this seem to rely on nvmem_cell_write(). This possibility can be handled
by nvmem_cell_prepare_write_buffer() because it allocates an
appropriately-sized heap buffer and avoids reading past the end of buf.

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

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

diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index 33ffa2aa4c11..74bf4d35a7a7 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -1767,8 +1767,7 @@ static int __nvmem_cell_entry_write(struct nvmem_cell_entry *cell, void *buf, si
 	struct nvmem_device *nvmem = cell->nvmem;
 	int rc;
 
-	if (!nvmem || nvmem->read_only ||
-	    (cell->bit_offset == 0 && len != cell->bytes))
+	if (!nvmem || nvmem->read_only)
 		return -EINVAL;
 
 	/*
@@ -1780,9 +1779,13 @@ static int __nvmem_cell_entry_write(struct nvmem_cell_entry *cell, void *buf, si
 		return -EINVAL;
 
 	if (cell->bit_offset || cell->nbits) {
+		if (len > cell->bytes)
+			return -EINVAL;
 		buf = nvmem_cell_prepare_write_buffer(cell, buf, len);
 		if (IS_ERR(buf))
 			return PTR_ERR(buf);
+	} else if (len != cell->bytes) {
+		return -EINVAL;
 	}
 
 	rc = nvmem_reg_write(nvmem, cell->offset, buf, cell->bytes);
-- 
2.46.2

[PATCH v2 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 74bf4d35a7a7..6f7aa2beb457 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -1816,6 +1816,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 v2 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 v2 1/3] nvmem: core: improve range check for nvmem_cell_write()

[Srinivas Kandagatla]

On 24/10/2024 16:40, Jennifer Berringer wrote:
> 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.
> 
thanks for spotting this, we should filter this out correctly.

> 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. This patch adds a check to avoid that problem
> and instead return -EINVAL when len is too large.
> 
> Rather than unconditionally checking that len exactly matches the nvmem
> cell size, allowing len to be smaller when bit shifts are involved may
> be helpful because some devices have nvmem cells that are less than 8
> bits but span two bytes, although no current devices or drivers that do
> this seem to rely on nvmem_cell_write(). This possibility can be handled
> by nvmem_cell_prepare_write_buffer() because it allocates an
> appropriately-sized heap buffer and avoids reading past the end of buf.
> 
> Fixes: 69aba7948cbe ("nvmem: Add a simple NVMEM framework for consumers")
> 
> Signed-off-by: Jennifer Berringer <jberring@redhat.com>
> ---
>   drivers/nvmem/core.c | 7 +++++--
>   1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
> index 33ffa2aa4c11..74bf4d35a7a7 100644
> --- a/drivers/nvmem/core.c
> +++ b/drivers/nvmem/core.c
> @@ -1767,8 +1767,7 @@ static int __nvmem_cell_entry_write(struct nvmem_cell_entry *cell, void *buf, si
>   	struct nvmem_device *nvmem = cell->nvmem;
>   	int rc;
>   
> -	if (!nvmem || nvmem->read_only ||
> -	    (cell->bit_offset == 0 && len != cell->bytes))
> +	if (!nvmem || nvmem->read_only)


if (!nvmem || nvmem->read_only || len != cell->bytes)
	return -EINVAL;

Does this work?

--srini
>   		return -EINVAL;
>   
>   	/*
> @@ -1780,9 +1779,13 @@ static int __nvmem_cell_entry_write(struct nvmem_cell_entry *cell, void *buf, si
>   		return -EINVAL;
>   
>   	if (cell->bit_offset || cell->nbits) {
> +		if (len > cell->bytes)
> +			return -EINVAL;
>   		buf = nvmem_cell_prepare_write_buffer(cell, buf, len);
>   		if (IS_ERR(buf))
>   			return PTR_ERR(buf);
> +	} else if (len != cell->bytes) {
> +		return -EINVAL;
>   	}
>   
>   	rc = nvmem_reg_write(nvmem, cell->offset, buf, cell->bytes);

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

[Jennifer Berringer]

On 10/29/24 13:55, Srinivas Kandagatla wrote:
> if (!nvmem || nvmem->read_only || len != cell->bytes)
>     return -EINVAL;
> 
> Does this work?
> 
> --srini

I decided against this because it seems potentially useful to allow len to be less than cell->bytes when bit_offset is nonzero. I assumed that was the purpose of the original "cell->bit_offset == 0".

For example, if a cell entry has the following field values
    { .bit_offset = 4, .nbits = 8, .bytes = 2, ...}
then it would make sense to call nvmem_cell_write() with len=1 in order to write 8 bits. To allow that, I used "len > cell->bytes" instead of "!=" later in this function:

>> @@ -1780,9 +1779,13 @@ static int __nvmem_cell_entry_write(struct nvmem_cell_entry *cell, void *buf, si
>>           return -EINVAL;
>>         if (cell->bit_offset || cell->nbits) {
>> +        if (len > cell->bytes)
>> +            return -EINVAL;
>>           buf = nvmem_cell_prepare_write_buffer(cell, buf, len);
>>           if (IS_ERR(buf))
>>               return PTR_ERR(buf);
>> +    } else if (len != cell->bytes) {
>> +        return -EINVAL;
>>       }

If you disagree with my reasoning then yes, your suggestion works and I can use that instead of what I wrote. None of the current in-tree callers of this function rely on that possibility I described.

Thank you for the feedback.

-Jennifer

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

[Srinivas Kandagatla]

On 29/10/2024 21:31, Jennifer Berringer wrote:
> On 10/29/24 13:55, Srinivas Kandagatla wrote:
>> if (!nvmem || nvmem->read_only || len != cell->bytes)
>>      return -EINVAL;
>>
>> Does this work?
>>
>> --srini
> 
> I decided against this because it seems potentially useful to allow len to be less than cell->bytes when bit_offset is nonzero. I assumed that was the purpose of the original "cell->bit_offset == 0".
> 
I don't think we support this case.

The reason why this check was initially added is,

If we have bit_offset as non zero or nbits set, cell->bytes is can be 
different to the actual space that is available in the cell, Ex: 2 bits 
with offset of 7 might end up taking 2 bytes. So the existing check is 
correct as it is and valid for cases where the bit_offset is 0.

In this particular case the right solution to the issue is to add more 
sanity checks in case bit_offset is non zero.


This change should help, can you pl  try it.

---------------------------->cut<-----------------------------
diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index 90c46f6e465d..e6d91a9a9dc5 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -1780,6 +1780,9 @@ static int __nvmem_cell_entry_write(struct 
nvmem_cell_entry *cell, void *buf, si
                 return -EINVAL;

         if (cell->bit_offset || cell->nbits) {
+               if (BITS_TO_BYTES(cell->nbits) != len)
+                       return -EINVAL;
+
                 buf = nvmem_cell_prepare_write_buffer(cell, buf, len);
                 if (IS_ERR(buf))
                         return PTR_ERR(buf);
---------------------------->cut<-----------------------------

thanks,
srini



> For example, if a cell entry has the following field values
>      { .bit_offset = 4, .nbits = 8, .bytes = 2, ...}
> then it would make sense to call nvmem_cell_write() with len=1 in order to write 8 bits. To allow that, I used "len > cell->bytes" instead of "!=" later in this function:
> 
>>> @@ -1780,9 +1779,13 @@ static int __nvmem_cell_entry_write(struct nvmem_cell_entry *cell, void *buf, si
>>>            return -EINVAL;
>>>          if (cell->bit_offset || cell->nbits) {
>>> +        if (len > cell->bytes)
>>> +            return -EINVAL;
>>>            buf = nvmem_cell_prepare_write_buffer(cell, buf, len);
>>>            if (IS_ERR(buf))
>>>                return PTR_ERR(buf);
>>> +    } else if (len != cell->bytes) {
>>> +        return -EINVAL;
>>>        }
> 
> If you disagree with my reasoning then yes, your suggestion works and I can use that instead of what I wrote. None of the current in-tree callers of this function rely on that possibility I described.
> 
> Thank you for the feedback.
> 
> -Jennifer
>