On Tue, 24 Sep 2024, Akilesh Kailash wrote:
> On Tue, Sep 24, 2024 at 11:45 AM Mikulas Patocka <mpatocka@xxxxxxxxxx> wrote:
> >
> >
> >
> > On Tue, 24 Sep 2024, Akilesh Kailash wrote:
> >
> > > On Tue, Sep 24, 2024 at 10:44 AM Sami Tolvanen <samitolvanen@xxxxxxxxxx> wrote:
> > > >
> > > > Hi Mikulas,
> > > >
> > > > On Tue, Sep 24, 2024 at 6:18 AM Mikulas Patocka <mpatocka@xxxxxxxxxx> wrote:
> > > > >
> > > > > Maxim Suhanov reported that dm-verity doesn't crash if an I/O error
> > > > > happens. In theory, this could be used to subvert security, because an
> > > > > attacker can create sectors that return error with the Write Uncorrectable
> > > > > command. Some programs may misbehave if they have to deal with EIO.
> > > >
> > > > I seem to recall that this was intentional. We used to restart/panic
> > > > on I/O errors with FEC enabled, but the behavior was changed in commit
> > > > 2c0468e054c0 ("dm verity: skip redundant verity_handle_err() on I/O
> > > > errors"). Akilesh, do you remember what exactly was the issue here?
> > > >
> > >
> > > I looked at the history and the original intent and the idea stems
> > > from this patch:
> > > https://lore.kernel.org/dm-devel/b004e7c7-f795-77ed-19b9-983785780e92@xxxxxxxxx/T/#mec4df1ba3f3cb63846875fb2bfc1f8b3100f31f1
> > >
> > > I think the EIO should be propagated back up the stack; I think this
> > > will trigger panic
> > > even for genuine I/O errors.
> >
> > Maxim Suhanov's point is that you should trigger panic for genuine I/O
> > errors. Otherwise the attacker could corrupt some sectors and let some
> > program receive EIO and the EIO could confuse the program to do something
> > insecure (for example, to not load a configuration file with security
> > policy).
> >
>
> Would it be ok to trigger the panic only when the system isn't shutting down?
> During shutdown, we may end up with EIO requests from the underlying
> driver and this may
> inadvertently trigger the panic.
>
> verity_is_system_shutting_down() should already have that logic to
> skip during the shutdown path.
OK.
I've added this logic.
Mikulas
