| Message ID | 1542073341-2843-1-git-send-email-liq3ea@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | memory: check write/read_with_attrs in memory dispatch | expand |
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 > >
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,
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(-)