Re: [Stable-8.1.1 11/34] softmmu: Assert data in bounds in iotlb_to_section

[Alex Bennée]

Michael Tokarev <mjt@tls.msk.ru> writes:

> 18.09.2023 12:19, Michael Tokarev wrote:
>> 09.09.2023 13:27, Michael Tokarev wrote:
>>> From: Richard Henderson <richard.henderson@linaro.org>
>>>
>>> Acked-by: Alex Bennée <alex.bennee@linaro.org>
>>> Suggested-by: Alex Bennée <alex.bennee@linaro.org>
>>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>>> (cherry picked from commit 86e4f93d827d3c1efd00cd8a906e38a2c0f2b5bc)
>>> Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
>>>
>>> diff --git a/softmmu/physmem.c b/softmmu/physmem.c
>>> index 3df73542e1..7597dc1c39 100644
>>> --- a/softmmu/physmem.c
>>> +++ b/softmmu/physmem.c
>>> @@ -2413,9 +2413,15 @@ MemoryRegionSection *iotlb_to_section(CPUState *cpu,
>>>       int asidx = cpu_asidx_from_attrs(cpu, attrs);
>>>       CPUAddressSpace *cpuas = &cpu->cpu_ases[asidx];
>>>       AddressSpaceDispatch *d = qatomic_rcu_read(&cpuas->memory_dispatch);
>>> -    MemoryRegionSection *sections = d->map.sections;
>>> +    int section_index = index & ~TARGET_PAGE_MASK;
>>> +    MemoryRegionSection *ret;
>>> +
>>> +    assert(section_index < d->map.sections_nb);
>> This assert now triggers on staging-8.1
>> https://ci.debian.net/data/autopkgtest/testing/amd64/d/dropbear/37993610/log.gz
>> https://ci.debian.net/data/autopkgtest/testing/amd64/c/cryptsetup/37993606/log.gz
>> 
>>> +    ret = d->map.sections + section_index;
>>> +    assert(ret->mr);
>>> +    assert(ret->mr->ops);
>>> -    return &sections[index & ~TARGET_PAGE_MASK];
>>> +    return ret;
>>>   }
>>>   static void io_mem_init(void)
>> In this upload I removed
>> softmmu-Use-async_run_on_cpu-in-tcg_commit.patch (0d58c660689f6da1),
>> and the test run uses tcg and -smp 4, which is the configuration which 
>> 0d58c6606
>> was supposed to fix.
>
> So, should this change not be in 8.1.1 too (together with 0d58c6606),
> or is it just the "messenger"?

Sorry my previous reply was eaten by my MUA.

The main purpose of the asserts is to catch corruption to the Memory
Regions early so we don't see weird failures later on (c.f. the 3
separate bugs for crashes in slightly different places).

IOW is we are crashing on the asserts in this patch but it's booting
without it we are just getting lucky.

>
> Or both should go?
>
> Today is the deadline day for 8.1.1.
>
> Thanks!
>
> /mjt


-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro