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
On Tue, Sep 24, 2024 at 11:45 AM Mikulas Patocka <mpatocka@redhat.com> wrote: > > > > 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). > 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. > Mikulas
On Tue, Sep 24, 2024 at 03:18:29PM +0200, Mikulas Patocka 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. > > 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. How does this interact with FEC, which can correct for I/O errors on the underlying media by reconstructing the data? > @@ -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"); The blk_status_t code that is checked above is usually the one that verity_verify_io() produced, not the original one. This could be an error from a cause other than an I/O error on the underlying media, like data corruption detected by dm-verity, or an error from the crypto API. Maybe what you are trying to do is really "panic if an I/O error would be returned to the layer above", not "panic if an I/O error occurred on the underlying media"? If so, this needs to be clearly explained. - Eric
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(-)