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.
