| Message ID | cad3f86f-09bf-bc5f-c086-fc623ad223f4@redhat.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Delegated to: | Mikulas Patocka |
| Headers | show |
| Series | dm-verity: restart or panic on an I/O error | expand |
Hi Mikulas, On Tue, Sep 24, 2024 at 6:18 AM Mikulas Patocka <mpatocka@redhat.com> 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? > This commit fixes dm-verity, so that if "panic_on_corruption" or > "restart_on_corruption" was specified and an I/O error happens, the > machine will panic or restart. > > This commit also changes kernel_restart to emergency_restart - > kernel_restart calls reboot notifiers and these reboot notifiers may wait > for the bio that failed. emergency_restart doesn't call the notifiers. > > Reported-by: Maxim Suhanov <dfirblog@gmail.com> > Signed-off-by: Mikulas Patocka <mpatocka@redhat.com> > Cc: stable@vger.kernel.org > > --- > drivers/md/dm-verity-target.c | 15 ++++++++++++++- > 1 file changed, 14 insertions(+), 1 deletion(-) > > Index: linux-2.6/drivers/md/dm-verity-target.c > =================================================================== > --- linux-2.6.orig/drivers/md/dm-verity-target.c 2024-09-23 17:48:08.000000000 +0200 > +++ linux-2.6/drivers/md/dm-verity-target.c 2024-09-24 11:34:08.000000000 +0200 > @@ -273,7 +273,7 @@ out: > return 0; > > if (v->mode == DM_VERITY_MODE_RESTART) > - kernel_restart("dm-verity device corrupted"); > + emergency_restart(); Can we still log the reason for the restart? I remember some folks used to rely on the "dm-verity device corrupted" message in the kernel log. Sami
On Tue, 24 Sep 2024, Sami Tolvanen wrote: > Hi Mikulas, > > On Tue, Sep 24, 2024 at 6:18 AM Mikulas Patocka <mpatocka@redhat.com> 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? > > Index: linux-2.6/drivers/md/dm-verity-target.c > > =================================================================== > > --- linux-2.6.orig/drivers/md/dm-verity-target.c 2024-09-23 17:48:08.000000000 +0200 > > +++ linux-2.6/drivers/md/dm-verity-target.c 2024-09-24 11:34:08.000000000 +0200 > > @@ -273,7 +273,7 @@ out: > > return 0; > > > > if (v->mode == DM_VERITY_MODE_RESTART) > > - kernel_restart("dm-verity device corrupted"); > > + emergency_restart(); > > Can we still log the reason for the restart? I remember some folks > used to rely on the "dm-verity device corrupted" message in the kernel > log. > > Sami OK. I've applied this on the top of my patch. Mikulas diff --git a/drivers/md/dm-verity-target.c b/drivers/md/dm-verity-target.c index 88012845bced..c9451df72c5a 100644 --- a/drivers/md/dm-verity-target.c +++ b/drivers/md/dm-verity-target.c @@ -272,8 +272,10 @@ static int verity_handle_err(struct dm_verity *v, enum verity_block_type type, if (v->mode == DM_VERITY_MODE_LOGGING) return 0; - if (v->mode == DM_VERITY_MODE_RESTART) + if (v->mode == DM_VERITY_MODE_RESTART) { + pr_emerg("dm-verity device corrupted\n"); emergency_restart(); + } if (v->mode == DM_VERITY_MODE_PANIC) panic("dm-verity device corrupted"); @@ -602,11 +604,13 @@ static void verity_finish_io(struct dm_verity_io *io, blk_status_t status) DMERR_LIMIT("%s has error: %s", v->data_dev->name, blk_status_to_str(status)); - if (v->mode == DM_VERITY_MODE_RESTART) + if (v->mode == DM_VERITY_MODE_RESTART) { + pr_emerg("dm-verity device corrupted\n"); emergency_restart(); + } if (v->mode == DM_VERITY_MODE_PANIC) - panic("dm-verity device has I/O error"); + panic("dm-verity device corrupted"); } bio_endio(bio);
On Tue, Sep 24, 2024 at 10:44 AM Sami Tolvanen <samitolvanen@google.com> wrote: > > Hi Mikulas, > > On Tue, Sep 24, 2024 at 6:18 AM Mikulas Patocka <mpatocka@redhat.com> 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@gmail.com/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. > > This commit fixes dm-verity, so that if "panic_on_corruption" or > > "restart_on_corruption" was specified and an I/O error happens, the > > machine will panic or restart. > > > > This commit also changes kernel_restart to emergency_restart - > > kernel_restart calls reboot notifiers and these reboot notifiers may wait > > for the bio that failed. emergency_restart doesn't call the notifiers. > > > > Reported-by: Maxim Suhanov <dfirblog@gmail.com> > > Signed-off-by: Mikulas Patocka <mpatocka@redhat.com> > > Cc: stable@vger.kernel.org > > > > --- > > drivers/md/dm-verity-target.c | 15 ++++++++++++++- > > 1 file changed, 14 insertions(+), 1 deletion(-) > > > > Index: linux-2.6/drivers/md/dm-verity-target.c > > =================================================================== > > --- linux-2.6.orig/drivers/md/dm-verity-target.c 2024-09-23 17:48:08.000000000 +0200 > > +++ linux-2.6/drivers/md/dm-verity-target.c 2024-09-24 11:34:08.000000000 +0200 > > @@ -273,7 +273,7 @@ out: > > return 0; > > > > if (v->mode == DM_VERITY_MODE_RESTART) > > - kernel_restart("dm-verity device corrupted"); > > + emergency_restart(); > > Can we still log the reason for the restart? I remember some folks > used to rely on the "dm-verity device corrupted" message in the kernel > log. > > Sami
On Tue, 24 Sep 2024, Akilesh Kailash wrote: > On Tue, Sep 24, 2024 at 10:44 AM Sami Tolvanen <samitolvanen@google.com> wrote: > > > > Hi Mikulas, > > > > On Tue, Sep 24, 2024 at 6:18 AM Mikulas Patocka <mpatocka@redhat.com> 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@gmail.com/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). Mikulas
Index: linux-2.6/drivers/md/dm-verity-target.c =================================================================== --- linux-2.6.orig/drivers/md/dm-verity-target.c 2024-09-23 17:48:08.000000000 +0200 +++ linux-2.6/drivers/md/dm-verity-target.c 2024-09-24 11:34:08.000000000 +0200 @@ -273,7 +273,7 @@ out: return 0; if (v->mode == DM_VERITY_MODE_RESTART) - kernel_restart("dm-verity device corrupted"); + emergency_restart(); if (v->mode == DM_VERITY_MODE_PANIC) panic("dm-verity device corrupted"); @@ -596,6 +596,19 @@ static void verity_finish_io(struct dm_v if (!static_branch_unlikely(&use_bh_wq_enabled) || !io->in_bh) verity_fec_finish_io(io); + if (unlikely(status != BLK_STS_OK) && unlikely(!(bio->bi_opf & REQ_RAHEAD))) { + if (v->mode == DM_VERITY_MODE_RESTART || + v->mode == DM_VERITY_MODE_PANIC) + DMERR_LIMIT("%s has error: %s", v->data_dev->name, + blk_status_to_str(status)); + + if (v->mode == DM_VERITY_MODE_RESTART) + emergency_restart(); + + if (v->mode == DM_VERITY_MODE_PANIC) + panic("dm-verity device has I/O error"); + } + bio_endio(bio); }
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. This commit fixes dm-verity, so that if "panic_on_corruption" or "restart_on_corruption" was specified and an I/O error happens, the machine will panic or restart. This commit also changes kernel_restart to emergency_restart - kernel_restart calls reboot notifiers and these reboot notifiers may wait for the bio that failed. emergency_restart doesn't call the notifiers. Reported-by: Maxim Suhanov <dfirblog@gmail.com> Signed-off-by: Mikulas Patocka <mpatocka@redhat.com> Cc: stable@vger.kernel.org --- drivers/md/dm-verity-target.c | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-)