Re: [PATCH v2 2/2] s390x/pci: Fix memory_region_access_valid call

[Cornelia Huck]

On Fri, 18 Dec 2020 10:37:38 +0100
Pierre Morel <pmorel@linux.ibm.com> wrote:

> On 12/17/20 11:16 PM, Matthew Rosato wrote:
> > In pcistb_service_handler, a call is made to validate that the memory
> > region can be accessed.  However, the call is made using the entire length
> > of the pcistb operation, which can be larger than the allowed memory
> > access size (8).  Since we already know that the provided buffer is a
> > multiple of 8, fix the call to memory_region_access_valid to iterate
> > over the memory region in the same way as the subsequent call to
> > memory_region_dispatch_write.
> > 
> > Fixes: 863f6f52b7 ("s390: implement pci instructions")
> > Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com>
> > ---
> >   hw/s390x/s390-pci-inst.c | 10 ++++++----
> >   1 file changed, 6 insertions(+), 4 deletions(-)
> > 
> > diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c
> > index e230293..76b08a3 100644
> > --- a/hw/s390x/s390-pci-inst.c
> > +++ b/hw/s390x/s390-pci-inst.c
> > @@ -821,10 +821,12 @@ int pcistb_service_call(S390CPU *cpu, uint8_t r1, 
> > uint8_t r3, uint64_t gaddr,
> >       mr = s390_get_subregion(mr, offset, len);
> >       offset -= mr->addr;
> >   
> > -    if (!memory_region_access_valid(mr, offset, len, true,
> > -                                    MEMTXATTRS_UNSPECIFIED)) {
> > -        s390_program_interrupt(env, PGM_OPERAND, ra);
> > -        return 0;
> > +    for (i = 0; i < len; i += 8) {
> > +        if (!memory_region_access_valid(mr, offset + i, 8, true,
> > +                                        MEMTXATTRS_UNSPECIFIED)) {
> > +            s390_program_interrupt(env, PGM_OPERAND, ra);
> > +            return 0;
> > +        }
> >       }
> >   
> >       if (s390_cpu_virt_mem_read(cpu, gaddr, ar, buffer, len)) {
> >   
> 
> wouldn't it be made automatically by defining the io_region 
> max_access_size when reading the bars in clp_service_call?
> 

But that's already what is happening, isn't it? The access check is
done for a size that is potentially too large, while the actual access
will happen in chunks of 8? I think that this patch is correct.