| Message ID | 20241024154050.3245228-2-jberring@redhat.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere, archived |
| Headers | show |
| Series | nvmem: fix out-of-bounds write | expand |
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);
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
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 >
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);
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(-)