On Mon, 30 Sep 2024, Will Drewry wrote:
> On Mon, Sep 30, 2024 at 1:07 PM Sami Tolvanen <samitolvanen@xxxxxxxxxx> wrote:
> >
> > On Mon, Sep 30, 2024 at 10:10 AM Will Drewry <wad@xxxxxxxxxxxx> wrote:
> > >
> > > On Mon, Sep 30, 2024 at 11:27 AM Mikulas Patocka <mpatocka@xxxxxxxxxx> wrote:
> > > >
> > > >
> > > >
> > > > On Mon, 30 Sep 2024, Will Drewry wrote:
> > > >
> > > > > > The dm-verity behavior was reported as a security bug, so by default, it
> > > > > > should behave in the secure way - i.e. restart or panic on I/O error.
> > > > > >
> > > > > > Do you intend to use dm-verity in Android and ChromeOS in the less-secure
> > > > > > way where it returns -EIO? Have you audited the Android and ChromeOS
> > > > > > codebase so that -EIO can't cause security breach? If yes, I can make a
> > > > > > configuration switch for you that will enable the old behavior.
> > > > >
> > > > > tl;dr don't change the default behavior, but adding a reboot-on-eio is nice.
> > > >
> > > > OK, so I can revert it if you want it.
> > >
> > > That or an argument so the old behavior can remain for those who are using it
> > > (I can send a patch for that if it's easier too).
> >
> > Thanks for chiming in on this, Will! I still think that adding a
> > separate restart_on_eio (or similar) flag would be the best solution.
> >
> > > > I'd like to ask - there is another change in that patch - I changed
> > > > kernel_restart("dm-verity device corrupted");
> > > > to
> > > > pr_emerg("dm-verity device corrupted\n");
> > > > emergency_restart();
> > > >
> > > > Because kernel_restart calls reboot notifiers and they may in theory wait
> > > > for the bio that caused the restart, resulting in deadlock.
> > > >
> > > > Do you want to have this part of the patch reverted too?
> > >
> > > IMHO, that's a good change! If the policy is to restart on corruption, then
> > > it makes sense to avoid the reboot notifiers.
> >
> > While I agree that this sounds good in principle, devices that use the
> > restart feature typically need to pass the reboot reason to a PMU, for
> > example, and it looks like Android devices depend on reboot notifiers
> > for this. Here's a recent Pixel implementation that checks for the
> > "dm-verity device corrupted" command as an example:
> >
> > https://android.googlesource.com/kernel/google-modules/power/reset/+/refs/heads/android-gs-comet-6.1-android15/pixel-zuma-reboot.c#81
>
> Good catch! (It's a shame it's using text as the API!) I totally
> spaced on that, and depending on where the corruption is, you mind end
> up in a boot loop.
If I add that 'reboot-on-eio' flag, should it also restart the kernel with
kernel_restart("dm-verity device corrupted")? Or, should it use a
different string?
Mikulas
