[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <a75f6b72-ea52-46a4-8790-13a4084d53b9@linaro.org>
Date: Wed, 30 Oct 2024 11:43:32 +0000
From: Srinivas Kandagatla <srinivas.kandagatla@...aro.org>
To: Jennifer Berringer <jberring@...hat.com>,
Sebastian Reichel <sre@...nel.org>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Maxime Ripard <mripard@...nel.org>
Cc: linux-kernel@...r.kernel.org, linux-pm@...r.kernel.org
Subject: Re: [PATCH v2 1/3] nvmem: core: improve range check for
nvmem_cell_write()
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
>
Powered by blists - more mailing lists