| Message ID | 5FA10ABA.1080109@huawei.com |
|---|---|
| State | Accepted |
| Commit | f25c7ca0cecb71428f864b9ccb6f128ec39ea94e |
| Headers | show |
| Series | target/microblaze: Fix possible array out of bounds in mmu_write() | expand |
On 03/11/2020 08.46, AlexChen wrote: > The size of env->mmu.regs is 3, but the range of 'rn' is [0, 5]. > To avoid data access out of bounds, only if 'rn' is less than 3, we > can print env->mmu.regs[rn]. In other cases, we can print > env->mmu.regs[MMU_R_TLBX]. ... since env->mmu.regs[MMU_R_TLBX] is used in the other cases in this function. Makes sense, indeed. > Reported-by: Euler Robot <euler.robot@huawei.com> > Signed-off-by: Alex Chen <alex.chen@huawei.com> > --- > target/microblaze/mmu.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/target/microblaze/mmu.c b/target/microblaze/mmu.c > index 1dbbb271c4..917ad6d69e 100644 > --- a/target/microblaze/mmu.c > +++ b/target/microblaze/mmu.c > @@ -234,7 +234,8 @@ void mmu_write(CPUMBState *env, bool ext, uint32_t rn, uint32_t v) > unsigned int i; > > qemu_log_mask(CPU_LOG_MMU, > - "%s rn=%d=%x old=%x\n", __func__, rn, v, env->mmu.regs[rn]); > + "%s rn=%d=%x old=%x\n", __func__, rn, v, > + rn < 3 ? env->mmu.regs[rn] : env->mmu.regs[MMU_R_TLBX]); > > if (cpu->cfg.mmu < 2 || !cpu->cfg.mmu_tlb_access) { > qemu_log_mask(LOG_GUEST_ERROR, "MMU access on MMU-less system\n"); > Reviewed-by: Thomas Huth <thuth@redhat.com>
On Tue, Nov 03, 2020 at 03:46:02PM +0800, AlexChen wrote: > The size of env->mmu.regs is 3, but the range of 'rn' is [0, 5]. > To avoid data access out of bounds, only if 'rn' is less than 3, we > can print env->mmu.regs[rn]. In other cases, we can print > env->mmu.regs[MMU_R_TLBX]. > > Reported-by: Euler Robot <euler.robot@huawei.com> > Signed-off-by: Alex Chen <alex.chen@huawei.com> Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com> > --- > target/microblaze/mmu.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/target/microblaze/mmu.c b/target/microblaze/mmu.c > index 1dbbb271c4..917ad6d69e 100644 > --- a/target/microblaze/mmu.c > +++ b/target/microblaze/mmu.c > @@ -234,7 +234,8 @@ void mmu_write(CPUMBState *env, bool ext, uint32_t rn, uint32_t v) > unsigned int i; > > qemu_log_mask(CPU_LOG_MMU, > - "%s rn=%d=%x old=%x\n", __func__, rn, v, env->mmu.regs[rn]); > + "%s rn=%d=%x old=%x\n", __func__, rn, v, > + rn < 3 ? env->mmu.regs[rn] : env->mmu.regs[MMU_R_TLBX]); > > if (cpu->cfg.mmu < 2 || !cpu->cfg.mmu_tlb_access) { > qemu_log_mask(LOG_GUEST_ERROR, "MMU access on MMU-less system\n"); > -- > 2.19.1
On 11/3/20 8:46 AM, AlexChen wrote: > The size of env->mmu.regs is 3, but the range of 'rn' is [0, 5]. > To avoid data access out of bounds, only if 'rn' is less than 3, we > can print env->mmu.regs[rn]. In other cases, we can print > env->mmu.regs[MMU_R_TLBX]. > > Reported-by: Euler Robot <euler.robot@huawei.com> > Signed-off-by: Alex Chen <alex.chen@huawei.com> > --- > target/microblaze/mmu.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/target/microblaze/mmu.c b/target/microblaze/mmu.c > index 1dbbb271c4..917ad6d69e 100644 > --- a/target/microblaze/mmu.c > +++ b/target/microblaze/mmu.c > @@ -234,7 +234,8 @@ void mmu_write(CPUMBState *env, bool ext, uint32_t rn, uint32_t v) > unsigned int i; > > qemu_log_mask(CPU_LOG_MMU, > - "%s rn=%d=%x old=%x\n", __func__, rn, v, env->mmu.regs[rn]); > + "%s rn=%d=%x old=%x\n", __func__, rn, v, > + rn < 3 ? env->mmu.regs[rn] : env->mmu.regs[MMU_R_TLBX]); Nack. If rn >= ARRAY_SIZE(env->mmu.regs), then don't displays it. Else it is confuse to see a value unrelated to the MMU index used... > > if (cpu->cfg.mmu < 2 || !cpu->cfg.mmu_tlb_access) { > qemu_log_mask(LOG_GUEST_ERROR, "MMU access on MMU-less system\n"); >
On 2020/11/6 22:16, Philippe Mathieu-Daudé wrote: > On 11/3/20 8:46 AM, AlexChen wrote: >> The size of env->mmu.regs is 3, but the range of 'rn' is [0, 5]. >> To avoid data access out of bounds, only if 'rn' is less than 3, we >> can print env->mmu.regs[rn]. In other cases, we can print >> env->mmu.regs[MMU_R_TLBX]. >> >> Reported-by: Euler Robot <euler.robot@huawei.com> >> Signed-off-by: Alex Chen <alex.chen@huawei.com> >> --- >> target/microblaze/mmu.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/target/microblaze/mmu.c b/target/microblaze/mmu.c >> index 1dbbb271c4..917ad6d69e 100644 >> --- a/target/microblaze/mmu.c >> +++ b/target/microblaze/mmu.c >> @@ -234,7 +234,8 @@ void mmu_write(CPUMBState *env, bool ext, uint32_t rn, uint32_t v) >> unsigned int i; >> >> qemu_log_mask(CPU_LOG_MMU, >> - "%s rn=%d=%x old=%x\n", __func__, rn, v, env->mmu.regs[rn]); >> + "%s rn=%d=%x old=%x\n", __func__, rn, v, >> + rn < 3 ? env->mmu.regs[rn] : env->mmu.regs[MMU_R_TLBX]); > > Nack. If rn >= ARRAY_SIZE(env->mmu.regs), then don't displays it. > Else it is confuse to see a value unrelated to the MMU index used... > Hi Philippe, Thanks for your review. The env->mmu.regs[MMU_R_TLBX] is used when rn >= ARRAY_SIZE(env->mmu.regs), can we change the description of the log as follows so that it doesn't confuse us? --- target/microblaze/mmu.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/target/microblaze/mmu.c b/target/microblaze/mmu.c index 1dbbb271c4..14863ed8d1 100644 --- a/target/microblaze/mmu.c +++ b/target/microblaze/mmu.c @@ -234,7 +234,9 @@ void mmu_write(CPUMBState *env, bool ext, uint32_t rn, uint32_t v) unsigned int i; qemu_log_mask(CPU_LOG_MMU, - "%s rn=%d=%x old=%x\n", __func__, rn, v, env->mmu.regs[rn]); + "%s rn=%d=%x %s=%x\n", __func__, rn, v, + rn < 3 ? "old" : "regs[MMU_R_TLBX]", + rn < 3 ? env->mmu.regs[rn] : env->mmu.regs[MMU_R_TLBX]); if (cpu->cfg.mmu < 2 || !cpu->cfg.mmu_tlb_access) { qemu_log_mask(LOG_GUEST_ERROR, "MMU access on MMU-less system\n"); Thanks, Alex
diff --git a/target/microblaze/mmu.c b/target/microblaze/mmu.c index 1dbbb271c4..917ad6d69e 100644 --- a/target/microblaze/mmu.c +++ b/target/microblaze/mmu.c @@ -234,7 +234,8 @@ void mmu_write(CPUMBState *env, bool ext, uint32_t rn, uint32_t v) unsigned int i; qemu_log_mask(CPU_LOG_MMU, - "%s rn=%d=%x old=%x\n", __func__, rn, v, env->mmu.regs[rn]); + "%s rn=%d=%x old=%x\n", __func__, rn, v, + rn < 3 ? env->mmu.regs[rn] : env->mmu.regs[MMU_R_TLBX]); if (cpu->cfg.mmu < 2 || !cpu->cfg.mmu_tlb_access) { qemu_log_mask(LOG_GUEST_ERROR, "MMU access on MMU-less system\n");
The size of env->mmu.regs is 3, but the range of 'rn' is [0, 5]. To avoid data access out of bounds, only if 'rn' is less than 3, we can print env->mmu.regs[rn]. In other cases, we can print env->mmu.regs[MMU_R_TLBX]. Reported-by: Euler Robot <euler.robot@huawei.com> Signed-off-by: Alex Chen <alex.chen@huawei.com> --- target/microblaze/mmu.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)