This can avoid the NULL-deref if the rm doesn't has a
read/write nor write/read_with_attrs callback.
Signed-off-by: Li Qiang <liq3ea@gmail.com>
---
memory.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/memory.c b/memory.c
index d14c6dec1d..3baf5857b9 100644
--- a/memory.c
+++ b/memory.c
@@ -1377,13 +1377,15 @@ static MemTxResult memory_region_dispatch_read1(MemoryRegion *mr,
mr->ops->impl.max_access_size,
memory_region_read_accessor,
mr, attrs);
- } else {
+ } else if (mr->ops->read_with_attrs) {
return access_with_adjusted_size(addr, pval, size,
mr->ops->impl.min_access_size,
mr->ops->impl.max_access_size,
memory_region_read_with_attrs_accessor,
mr, attrs);
}
+
+ return MEMTX_DECODE_ERROR;
}
MemTxResult memory_region_dispatch_read(MemoryRegion *mr,
@@ -1454,7 +1456,7 @@ MemTxResult memory_region_dispatch_write(MemoryRegion *mr,
mr->ops->impl.max_access_size,
memory_region_write_accessor, mr,
attrs);
- } else {
+ } else if (mr->ops->write_with_attrs) {
return
access_with_adjusted_size(addr, &data, size,
mr->ops->impl.min_access_size,
@@ -1462,6 +1464,8 @@ MemTxResult memory_region_dispatch_write(MemoryRegion *mr,
memory_region_write_with_attrs_accessor,
mr, attrs);
}
+
+ return MEMTX_DECODE_ERROR;
}
void memory_region_init_io(MemoryRegion *mr,
--
2.11.0
On 13 November 2018 at 01:42, Li Qiang <liq3ea@gmail.com> wrote: > This can avoid the NULL-deref if the rm doesn't has a > read/write nor write/read_with_attrs callback. > > Signed-off-by: Li Qiang <liq3ea@gmail.com> > --- > memory.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) Alternative approach -- assert that every MemoryRegionOps has pointers to callbacks in it, when it is registered in memory_region_init_io() and memory_region_init_rom_device_nomigrate(). I don't have a strong opinion on which is better, but I guess I slightly favour requiring devices to be specific about what their read/write behaviour is. Do we have many devices that legitimately only want to implement one of read and write, not both ? thanks -- PMM
Peter Maydell <peter.maydell@linaro.org> 于2018年11月13日周二 下午5:49写道: > On 13 November 2018 at 01:42, Li Qiang <liq3ea@gmail.com> wrote: > > This can avoid the NULL-deref if the rm doesn't has a > > read/write nor write/read_with_attrs callback. > > > > Signed-off-by: Li Qiang <liq3ea@gmail.com> > > --- > > memory.c | 8 ++++++-- > > 1 file changed, 6 insertions(+), 2 deletions(-) > > Alternative approach -- assert that every MemoryRegionOps has > pointers to callbacks in it, when it is registered in > memory_region_init_io() > Actually I have considered this approach, but I rember Paolo remind me that some of this MR without read callback's is valid because the read can never be called, such as 'notdirty_mem_ops'. So I choose add a check here. > and memory_region_init_rom_device_nomigrate(). > > I don't have a strong opinion on which is better, but I guess > I slightly favour requiring devices to be specific about what > their read/write behaviour is. > > Do we have many devices that legitimately only want to implement > one of read and write, not both ? AFAICS, the device has just one read or write is not uncommon. But nearly all of them implement a nop function does nothing. So there just very little lack of the read or write function. Thanks, Li Qiang > > thanks > -- PMM >
Ping... It makes sense as when we use 'memory_region_read_accessor' we check mr->ops->read. but when we use 'memory_region_read_with_attrs_accessor', we doesn't check this. Thanks, Li Qiang Li Qiang <liq3ea@gmail.com> 于2018年11月13日周二 上午9:42写道: > This can avoid the NULL-deref if the rm doesn't has a > read/write nor write/read_with_attrs callback. > > Signed-off-by: Li Qiang <liq3ea@gmail.com> > --- > memory.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/memory.c b/memory.c > index d14c6dec1d..3baf5857b9 100644 > --- a/memory.c > +++ b/memory.c > @@ -1377,13 +1377,15 @@ static MemTxResult > memory_region_dispatch_read1(MemoryRegion *mr, > mr->ops->impl.max_access_size, > memory_region_read_accessor, > mr, attrs); > - } else { > + } else if (mr->ops->read_with_attrs) { > return access_with_adjusted_size(addr, pval, size, > mr->ops->impl.min_access_size, > mr->ops->impl.max_access_size, > > memory_region_read_with_attrs_accessor, > mr, attrs); > } > + > + return MEMTX_DECODE_ERROR; > } > > MemTxResult memory_region_dispatch_read(MemoryRegion *mr, > @@ -1454,7 +1456,7 @@ MemTxResult > memory_region_dispatch_write(MemoryRegion *mr, > mr->ops->impl.max_access_size, > memory_region_write_accessor, mr, > attrs); > - } else { > + } else if (mr->ops->write_with_attrs) { > return > access_with_adjusted_size(addr, &data, size, > mr->ops->impl.min_access_size, > @@ -1462,6 +1464,8 @@ MemTxResult > memory_region_dispatch_write(MemoryRegion *mr, > > memory_region_write_with_attrs_accessor, > mr, attrs); > } > + > + return MEMTX_DECODE_ERROR; > } > > void memory_region_init_io(MemoryRegion *mr, > -- > 2.11.0 > >
© 2016 - 2024 Red Hat, Inc.