[Devel] dm: ploop: arithemtic overflow in ploop

[Evgenii Shatokhin]

Hi,

On 10.05.2024 16:15, Alexey Kuznetsov wrote:
> May be we do. I do such mistakes all the time, as I am paranoid about
> saving memory, using minimal possible type.
> I checked code since this morning, this is the only place, which I
> found. And it fixed this urgent problem.
> Full audit can be done later, after everyone patched and damage is isolated.
> 
> General note. The code looks very good. It needs some testing, as we
> see, but it is good and this fiasco
> must not be interpreted as a sign of badness.

Is it possible to rebuild this particular kernel with UBSan ("undefined 
behaviour sanitizer") enabled, just for testing and debugging? It could 
probably help find other overflow and shift-related issues, if there are 
any.

(And if the tests trigger the respective code paths, of course.)

> 
> On Fri, May 10, 2024 at 9:09 PM Denis V. Lunev <den at virtuozzo.com> wrote:
>>
>> On 5/10/24 14:54, Alexey Kuznetsov wrote:
>>> Images of size > 2TB are corrupted!
>>>
>>> https://pmc.acronis.work/browse/TTASK-68430
>>>
>>> Signed-off-by: Alexey Kuznetsov <kuznet at acronis.com>
>>> ---
>>>    drivers/md/dm-ploop.h | 2 +-
>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/md/dm-ploop.h b/drivers/md/dm-ploop.h
>>> index db36687..e693d0e 100644
>>> --- a/drivers/md/dm-ploop.h
>>> +++ b/drivers/md/dm-ploop.h
>>> @@ -317,7 +317,7 @@ static inline void ploop_remap_to_cluster(struct ploop *ploop,
>>>                                          struct pio *pio, u32 clu)
>>>    {
>>>        pio->bi_iter.bi_sector &= ((1 << ploop->cluster_log) - 1);
>>> -     pio->bi_iter.bi_sector |= (clu << ploop->cluster_log);
>>> +     pio->bi_iter.bi_sector |= ((u64)clu << ploop->cluster_log);
>>>    }
>>>
>>>    static inline bool ploop_whole_cluster(struct ploop *ploop, struct pio *pio)
>> this is really integer overflow. Validated that with the simple
>> test program.
>>
>> iris ~ $ cat 1.c
>> #include <stdint.h>
>> #include <stdio.h>
>>
>> struct s
>> {
>>       uint64_t x;
>> };
>>
>> int main()
>> {
>>       uint32_t clu = 0x200000, log = 20;
>>       struct s st = {
>>           .x = 0,
>>       };
>>
>>       st.x |= clu << log;
>>       printf("%lu\n", st.x);
>>       st.x |= (uint64_t)clu << log;
>>       printf("%lu\n", st.x);
>>       return 0;
>> }
>> iris ~ $ ./a.out
>> 0
>> 2199023255552
>> iris ~ $
>>
>> The most important question is that do we have other similar
>> places or not?
>>
>> Den
> 

Regards,
Evgenii