Re: [PATCH 4/9] fibmap: Use bmap instead of ->bmap method in ioctl_fibmap

[Carlos Maiolino <cmaiolino@redhat.com>]

Meh... Sorry andreas, your reply became disconnected from the thread, and I
think I didn't reply.

On Thu, Aug 08, 2019 at 12:53:25PM -0600, Andreas Dilger wrote:
> On Aug 8, 2019, at 1:12 AM, Carlos Maiolino <cmaiolino@redhat.com> wrote:
> > 
> >>> 
> >>>> Maybe I am not seeing something or having a different thinking you have, but
> >>>> this is the behavior we have now, without my patches. And we can't really change
> >>>> it; the user view of this implementation.
> >>>> That's why I didn't try to change the result, so the truncation still happens.
> >>> 
> >>> I understand that we're not generally supposed to change existing
> >>> userspace interfaces, but the fact remains that allowing truncated
> >>> responses causes *filesystem corruption*.
> >>> 
> >>> We know that the most well known FIBMAP callers are bootloaders, and we
> >>> know what they do with the information they get -- they use it to record
> >>> the block map of boot files.  So if the IPL/grub/whatever installer
> >>> queries the boot file and the boot file is at block 12345678901 (a
> >>> 34-bit number), this interface truncates that to 3755744309 (a 32-bit
> >>> number) and that's where the bootloader will think its boot files are.
> >>> The installation succeeds, the user reboots and *kaboom* the system no
> >>> longer boots because the contents of block 3755744309 is not a bootloader.
> >>> 
> >>> Worse yet, grub1 used FIBMAP data to record the location of the grub
> >>> environment file and installed itself between the MBR and the start of
> >>> partition 1.  If the environment file is at offset 1234578901, grub will
> >>> write status data to its environment file (which it thinks is at
> >>> 3755744309) and *KABOOM* we've just destroyed whatever was in that
> >>> block.
> >>> 
> >>> Far better for the bootloader installation script to hit an error and
> >>> force the admin to deal with the situation than for the system to become
> >>> unbootable.  That's *why* the (newer) iomap bmap implementation does not
> >>> return truncated mappings, even though the classic implementation does.
> >>> 
> >>> The classic code returning truncated results is a broken behavior.
> >> 
> >> How long as it been broken for? And if we do fix it, I'd just like for
> >> a nice commit lot describing potential risks of not applying it. *If*
> >> the issue exists as-is today, the above contains a lot of information
> >> for addressing potential issues, even if theoretical.
> >> 
> > 
> > It's broken since forever. This has always been the FIBMAP behavior.
> 
> It's been broken since forever, but only for filesystems larger than 4TB or
> 16TB (2^32 blocks), which are only becoming commonplace for root disks recently.
> Also, doesn't LILO have a limit on the location of the kernel image, in the
> first 1GB or similar?
> 
> So maybe this is not an issue that FIBMAP users ever hit in practise anyway,
> but I agree that it doesn't make sense to return bad data (32-bit wrapped block
> numbers) and 0 should be returned in such cases.
> 

Thanks for the input, but TBH I don't use LILO for a long time, and I don't
remember exactly how it works.

Anyway, I have 2 bugs to fix in this code, after I can get this series in, one
is the overflow we'll probably need kernel-api approval, and another one is the
acceptance of negative values into FIBMAP, which we have no protection at all.
I'll fix both once I can get the main series in.

Cheers


> 
> Cheers, Andreas
> 
> 
> 
> 
> 



-- 
Carlos