| Message ID | 20210212104022.GA242669@embeddedor |
|---|---|
| State | Accepted |
| Headers | show |
| Series | mtd: physmap: physmap-bt1-rom: Fix unintentional stack access | expand |
Hi Gustavo, "Gustavo A. R. Silva" <gustavoars@kernel.org> wrote on Fri, 12 Feb 2021 04:40:22 -0600: > Cast &data to (char *) in order to avoid unintentionally accessing > the stack. > > Notice that data is of type u32, so any increment to &data > will be in the order of 4-byte chunks, and this piece of code > is actually intended to be a byte offset. I don't have the same reading. I don't say that Coverity report is wrong, but let's discuss this a bit further. Given that &data is of type u32 *, you say that "&data + shift" produces increments of 4-bytes, ie. we would access "&data + 4 * shift"? Because I don't think this is the case (again, I may be wrong). I'm sure this would be the case if we dereferenced data like data[shift] though, but in this case I don't see what this cast is fixing. Can you enlighten me? Could the out-of-bounds warning come from the fact that shift might be bigger than the data array spread? > Fixes: b3e79e7682e0 ("mtd: physmap: Add Baikal-T1 physically mapped ROM support") > Addresses-Coverity-ID: 1497765 ("Out-of-bounds access") > Cc: stable@vger.kernel.org > Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org> > --- > drivers/mtd/maps/physmap-bt1-rom.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/mtd/maps/physmap-bt1-rom.c b/drivers/mtd/maps/physmap-bt1-rom.c > index a35450002284..58782cfaf71c 100644 > --- a/drivers/mtd/maps/physmap-bt1-rom.c > +++ b/drivers/mtd/maps/physmap-bt1-rom.c > @@ -79,7 +79,7 @@ static void __xipram bt1_rom_map_copy_from(struct map_info *map, > if (shift) { > chunk = min_t(ssize_t, 4 - shift, len); > data = readl_relaxed(src - shift); > - memcpy(to, &data + shift, chunk); > + memcpy(to, (char *)&data + shift, chunk); > src += chunk; > to += chunk; > len -= chunk; Thanks, Miquèl
On 2/12/21 08:12, Miquel Raynal wrote: > Hi Gustavo, > > "Gustavo A. R. Silva" <gustavoars@kernel.org> wrote on Fri, 12 Feb 2021 > 04:40:22 -0600: > >> Cast &data to (char *) in order to avoid unintentionally accessing >> the stack. >> >> Notice that data is of type u32, so any increment to &data >> will be in the order of 4-byte chunks, and this piece of code >> is actually intended to be a byte offset. > > I don't have the same reading. I don't say that Coverity report is > wrong, but let's discuss this a bit further. > > Given that &data is of type u32 *, you say that "&data + shift" > produces increments of 4-bytes, ie. we would access "&data + 4 * > shift"? Because I don't think this is the case (again, I may be wrong). Yep; this is pointer arithmetic. If you have an object ptr of type u32 *: u32 *ptr; and let's say it points to address 100. If you increment it by one: ptr++ ptr will now point to address 104, not to 101. Now, if instead, you first cast ptr to 'char *' and increment it by 1, then it will point to address 101. > > I'm sure this would be the case if we dereferenced data like > data[shift] though, but in this case I don't see what this cast is > fixing. Can you enlighten me? > > Could the out-of-bounds warning come from the fact that shift > might be bigger than the data array spread? Whats Coverity wants to say here is basically that using &data as an array might corrupt adjacent memory. Thanks -- Gustavo > >> Fixes: b3e79e7682e0 ("mtd: physmap: Add Baikal-T1 physically mapped ROM support") >> Addresses-Coverity-ID: 1497765 ("Out-of-bounds access") >> Cc: stable@vger.kernel.org >> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org> >> --- >> drivers/mtd/maps/physmap-bt1-rom.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/mtd/maps/physmap-bt1-rom.c b/drivers/mtd/maps/physmap-bt1-rom.c >> index a35450002284..58782cfaf71c 100644 >> --- a/drivers/mtd/maps/physmap-bt1-rom.c >> +++ b/drivers/mtd/maps/physmap-bt1-rom.c >> @@ -79,7 +79,7 @@ static void __xipram bt1_rom_map_copy_from(struct map_info *map, >> if (shift) { >> chunk = min_t(ssize_t, 4 - shift, len); >> data = readl_relaxed(src - shift); >> - memcpy(to, &data + shift, chunk); >> + memcpy(to, (char *)&data + shift, chunk); >> src += chunk; >> to += chunk; >> len -= chunk; > > Thanks, > Miquèl >
Hi Gustavo, "Gustavo A. R. Silva" <gustavo@embeddedor.com> wrote on Fri, 12 Feb 2021 08:45:33 -0600: > On 2/12/21 08:12, Miquel Raynal wrote: > > Hi Gustavo, > > > > "Gustavo A. R. Silva" <gustavoars@kernel.org> wrote on Fri, 12 Feb 2021 > > 04:40:22 -0600: > > > >> Cast &data to (char *) in order to avoid unintentionally accessing > >> the stack. > >> > >> Notice that data is of type u32, so any increment to &data > >> will be in the order of 4-byte chunks, and this piece of code > >> is actually intended to be a byte offset. > > > > I don't have the same reading. I don't say that Coverity report is > > wrong, but let's discuss this a bit further. > > > > Given that &data is of type u32 *, you say that "&data + shift" > > produces increments of 4-bytes, ie. we would access "&data + 4 * > > shift"? Because I don't think this is the case (again, I may be wrong). > > Yep; this is pointer arithmetic. If you have an object ptr of type u32 *: > > u32 *ptr; > > and let's say it points to address 100. If you increment it by one: > > ptr++ > > ptr will now point to address 104, not to 101. > > Now, if instead, you first cast ptr to 'char *' and increment it by 1, > then it will point to address 101. Yep, I got confused with the proper addition compared to dereferencing. Patch looks legitimate. Thanks, Miquèl
On 2/12/21 08:50, Miquel Raynal wrote: >> Now, if instead, you first cast ptr to 'char *' and increment it by 1, >> then it will point to address 101. > > Yep, I got confused with the proper addition compared to dereferencing. No problem. > Patch looks legitimate. Great. :) Thanks -- Gustavo
On Fri, Feb 12, 2021 at 04:40:22AM -0600, Gustavo A. R. Silva wrote: > Cast &data to (char *) in order to avoid unintentionally accessing > the stack. > > Notice that data is of type u32, so any increment to &data > will be in the order of 4-byte chunks, and this piece of code > is actually intended to be a byte offset. Thanks, one more time.) Acked-by: Serge Semin <fancer.lancer@gmail.com> > > Fixes: b3e79e7682e0 ("mtd: physmap: Add Baikal-T1 physically mapped ROM support") > Addresses-Coverity-ID: 1497765 ("Out-of-bounds access") > Cc: stable@vger.kernel.org > Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org> > --- > drivers/mtd/maps/physmap-bt1-rom.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/mtd/maps/physmap-bt1-rom.c b/drivers/mtd/maps/physmap-bt1-rom.c > index a35450002284..58782cfaf71c 100644 > --- a/drivers/mtd/maps/physmap-bt1-rom.c > +++ b/drivers/mtd/maps/physmap-bt1-rom.c > @@ -79,7 +79,7 @@ static void __xipram bt1_rom_map_copy_from(struct map_info *map, > if (shift) { > chunk = min_t(ssize_t, 4 - shift, len); > data = readl_relaxed(src - shift); > - memcpy(to, &data + shift, chunk); > + memcpy(to, (char *)&data + shift, chunk); > src += chunk; > to += chunk; > len -= chunk; > -- > 2.27.0 >
On Fri, 2021-02-12 at 10:40:22 UTC, "Gustavo A. R. Silva" wrote: > Cast &data to (char *) in order to avoid unintentionally accessing > the stack. > > Notice that data is of type u32, so any increment to &data > will be in the order of 4-byte chunks, and this piece of code > is actually intended to be a byte offset. > > Fixes: b3e79e7682e0 ("mtd: physmap: Add Baikal-T1 physically mapped ROM support") > Addresses-Coverity-ID: 1497765 ("Out-of-bounds access") > Cc: stable@vger.kernel.org > Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org> > Acked-by: Serge Semin <fancer.lancer@gmail.com> Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git mtd/next, thanks. Miquel
diff --git a/drivers/mtd/maps/physmap-bt1-rom.c b/drivers/mtd/maps/physmap-bt1-rom.c index a35450002284..58782cfaf71c 100644 --- a/drivers/mtd/maps/physmap-bt1-rom.c +++ b/drivers/mtd/maps/physmap-bt1-rom.c @@ -79,7 +79,7 @@ static void __xipram bt1_rom_map_copy_from(struct map_info *map, if (shift) { chunk = min_t(ssize_t, 4 - shift, len); data = readl_relaxed(src - shift); - memcpy(to, &data + shift, chunk); + memcpy(to, (char *)&data + shift, chunk); src += chunk; to += chunk; len -= chunk;
Cast &data to (char *) in order to avoid unintentionally accessing the stack. Notice that data is of type u32, so any increment to &data will be in the order of 4-byte chunks, and this piece of code is actually intended to be a byte offset. Fixes: b3e79e7682e0 ("mtd: physmap: Add Baikal-T1 physically mapped ROM support") Addresses-Coverity-ID: 1497765 ("Out-of-bounds access") Cc: stable@vger.kernel.org Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org> --- drivers/mtd/maps/physmap-bt1-rom.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)