Re: [PATCH][next] nvmem: core: Fix unintentional sign extension issue
On 11/03/2021 09:53, Colin King wrote: From: Colin Ian King The shifting of the u8 integer buf[3] by 24 bits to the left will be promoted to a 32 bit signed int and then sign-extended to a u64. In the event that the top bit of buf[3] is set then all then all the upper 32 bits of the u64 end up as also being set because of the sign-extension. Fix this by casting buf[i] to a u64 before the shift. Addresses-Coverity: ("Unintended sign extension") Fixes: 097eb1136ebb ("nvmem: core: Add functions to make number reading easy") Signed-off-by: Colin Ian King --- Applied thanks, --srini drivers/nvmem/core.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c index 635e3131eb5f..bca671ff4e54 100644 --- a/drivers/nvmem/core.c +++ b/drivers/nvmem/core.c @@ -1693,7 +1693,7 @@ int nvmem_cell_read_variable_le_u64(struct device *dev, const char *cell_id, /* Copy w/ implicit endian conversion */ *val = 0; for (i = 0; i < len; i++) - *val |= buf[i] << (8 * i); + *val |= (uint64_t)buf[i] << (8 * i); kfree(buf);
Re: [PATCH][next] nvmem: core: Fix unintentional sign extension issue
On 11/03/2021 17:12, Doug Anderson wrote: > Hi, > > On Thu, Mar 11, 2021 at 1:53 AM Colin King wrote: >> >> From: Colin Ian King >> >> The shifting of the u8 integer buf[3] by 24 bits to the left will >> be promoted to a 32 bit signed int and then sign-extended to a >> u64. In the event that the top bit of buf[3] is set then all >> then all the upper 32 bits of the u64 end up as also being set >> because of the sign-extension. Fix this by casting buf[i] to >> a u64 before the shift. >> >> Addresses-Coverity: ("Unintended sign extension") >> Fixes: 097eb1136ebb ("nvmem: core: Add functions to make number reading >> easy") >> Signed-off-by: Colin Ian King >> --- >> drivers/nvmem/core.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) > > Thanks! I had only tested the "u64" version to read smaller data and > store it in a u64. From my understanding of C rules, without your > patch it would have been even worse than just a sign extension though, > right? Shifting "buf[i]" by more than 32 bits would just not have > worked right. yep, that's correct, I forgot to mention that issue too :-/ > > In any case: > > Reviewed-by: Douglas Anderson >
Re: [PATCH][next] nvmem: core: Fix unintentional sign extension issue
Hi, On Thu, Mar 11, 2021 at 1:53 AM Colin King wrote: > > From: Colin Ian King > > The shifting of the u8 integer buf[3] by 24 bits to the left will > be promoted to a 32 bit signed int and then sign-extended to a > u64. In the event that the top bit of buf[3] is set then all > then all the upper 32 bits of the u64 end up as also being set > because of the sign-extension. Fix this by casting buf[i] to > a u64 before the shift. > > Addresses-Coverity: ("Unintended sign extension") > Fixes: 097eb1136ebb ("nvmem: core: Add functions to make number reading easy") > Signed-off-by: Colin Ian King > --- > drivers/nvmem/core.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Thanks! I had only tested the "u64" version to read smaller data and store it in a u64. From my understanding of C rules, without your patch it would have been even worse than just a sign extension though, right? Shifting "buf[i]" by more than 32 bits would just not have worked right. In any case: Reviewed-by: Douglas Anderson
[PATCH][next] nvmem: core: Fix unintentional sign extension issue
From: Colin Ian King The shifting of the u8 integer buf[3] by 24 bits to the left will be promoted to a 32 bit signed int and then sign-extended to a u64. In the event that the top bit of buf[3] is set then all then all the upper 32 bits of the u64 end up as also being set because of the sign-extension. Fix this by casting buf[i] to a u64 before the shift. Addresses-Coverity: ("Unintended sign extension") Fixes: 097eb1136ebb ("nvmem: core: Add functions to make number reading easy") Signed-off-by: Colin Ian King --- drivers/nvmem/core.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c index 635e3131eb5f..bca671ff4e54 100644 --- a/drivers/nvmem/core.c +++ b/drivers/nvmem/core.c @@ -1693,7 +1693,7 @@ int nvmem_cell_read_variable_le_u64(struct device *dev, const char *cell_id, /* Copy w/ implicit endian conversion */ *val = 0; for (i = 0; i < len; i++) - *val |= buf[i] << (8 * i); + *val |= (uint64_t)buf[i] << (8 * i); kfree(buf); -- 2.30.2