[patch] New mode DM-Verity error handling

[patch] New mode DM-Verity error handling

[JeongHyeon Lee]

[-- Attachment #1: Type: text/plain, Size: 809 bytes --]

Hello, Dear devcice-mapper maintainers.

I'm JeongHyeon Lee, work in Samsung. I'm chage of DM-Verity feature with 
Mr. sunwook eom.
I have a patch or suggestion about DM-Verity error handling.

Our device (smart phone) need DM-Verity feature. So I hope there is new 
mode DM-Verity error handling.
This new mode concept is When detect corrupted block, will be go to panic.

Because our team policy is found device DM-Verity error, device will go 
panic.
And then analyze what kind of device fault (crash UFS, IO error, DRAM 
bit flip etc)

In addition to the smart phone, I would like to have an option that 
users or administrators can use accordingly.
There are patch contents in the attachment. I would really appreciate it 
if you could check it.

I will look forward to hearing from yours.
Thank you :)


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-dm-verity-new-error-handling-mode-for-corrupted-bloc.patch --]
[-- Type: text/x-patch; name="0001-dm-verity-new-error-handling-mode-for-corrupted-bloc.patch", Size: 3309 bytes --]

From 6d3e508ed6872bfdc88d6ad979ac5c0347144fbb Mon Sep 17 00:00:00 2001
From: "jhs2.lee" <jhs2.lee@samsung.com>
Date: Thu, 18 Jun 2020 15:32:20 +0900
Subject: [PATCH] dm verity: new error handling mode for corrupted blocks

There is no panic error handling mode when a problem occurs.
So We add new error handling mode. users and administrators
setup to fit your need.

Signed-off-by: jhs2.lee <jhs2.lee@samsung.com>
---
 Documentation/admin-guide/device-mapper/verity.rst |  4 ++++
 drivers/md/dm-verity-target.c                      | 11 +++++++++++
 drivers/md/dm-verity.h                             |  3 ++-
 3 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/Documentation/admin-guide/device-mapper/verity.rst b/Documentation/admin-guide/device-mapper/verity.rst
index bb02caa45289..66f71f0dab1b 100644
--- a/Documentation/admin-guide/device-mapper/verity.rst
+++ b/Documentation/admin-guide/device-mapper/verity.rst
@@ -83,6 +83,10 @@ restart_on_corruption
     not compatible with ignore_corruption and requires user space support to
     avoid restart loops.
 
+panic_on_corruption
+    Panic the device when a corrupted block is discovered. This option is
+    not compatible with ignore_corruption and restart_on_corruption.
+
 ignore_zero_blocks
     Do not verify blocks that are expected to contain zeroes and always return
     zeroes instead. This may be useful if the partition contains unused blocks
diff --git a/drivers/md/dm-verity-target.c b/drivers/md/dm-verity-target.c
index eec9f252e935..c89114e7886c 100644
--- a/drivers/md/dm-verity-target.c
+++ b/drivers/md/dm-verity-target.c
@@ -30,6 +30,7 @@
 
 #define DM_VERITY_OPT_LOGGING		"ignore_corruption"
 #define DM_VERITY_OPT_RESTART		"restart_on_corruption"
+#define DM_VERITY_OPT_PANIC		"panic_on_corruption"
 #define DM_VERITY_OPT_IGN_ZEROES	"ignore_zero_blocks"
 #define DM_VERITY_OPT_AT_MOST_ONCE	"check_at_most_once"
 
@@ -254,6 +255,9 @@ static int verity_handle_err(struct dm_verity *v, enum verity_block_type type,
 	if (v->mode == DM_VERITY_MODE_RESTART)
 		kernel_restart("dm-verity device corrupted");
 
+	if (v->mode == DM_VERITY_MODE_PANIC)
+		panic("dm-verity device corrupted");
+
 	return 1;
 }
 
@@ -742,6 +746,9 @@ static void verity_status(struct dm_target *ti, status_type_t type,
 			case DM_VERITY_MODE_RESTART:
 				DMEMIT(DM_VERITY_OPT_RESTART);
 				break;
+			case DM_VERITY_MODE_PANIC:
+				DMEMIT(DM_VERITY_OPT_PANIC);
+				break;
 			default:
 				BUG();
 			}
@@ -907,6 +914,10 @@ static int verity_parse_opt_args(struct dm_arg_set *as, struct dm_verity *v,
 			v->mode = DM_VERITY_MODE_RESTART;
 			continue;
 
+		} else if (!strcasecmp(arg_name, DM_VERITY_OPT_PANIC)) {
+			v->mode = DM_VERITY_MODE_PANIC;
+			continue;
+
 		} else if (!strcasecmp(arg_name, DM_VERITY_OPT_IGN_ZEROES)) {
 			r = verity_alloc_zero_digest(v);
 			if (r) {
diff --git a/drivers/md/dm-verity.h b/drivers/md/dm-verity.h
index 641b9e3a399b..4e769d13473a 100644
--- a/drivers/md/dm-verity.h
+++ b/drivers/md/dm-verity.h
@@ -20,7 +20,8 @@
 enum verity_mode {
 	DM_VERITY_MODE_EIO,
 	DM_VERITY_MODE_LOGGING,
-	DM_VERITY_MODE_RESTART
+	DM_VERITY_MODE_RESTART,
+	DM_VERITY_MODE_PANIC
 };
 
 enum verity_block_type {
-- 
2.17.1

Re: New mode DM-Verity error handling

[Mike Snitzer]

On Thu, Jun 18 2020 at  2:56am -0400,
JeongHyeon Lee <jhs2.lee@samsung.com> wrote:

> Hello, Dear devcice-mapper maintainers.
> 
> I'm JeongHyeon Lee, work in Samsung. I'm chage of DM-Verity feature with 
> Mr. sunwook eom.
> I have a patch or suggestion about DM-Verity error handling.
> 
> Our device (smart phone) need DM-Verity feature. So I hope there is new 
> mode DM-Verity error handling.
> This new mode concept is When detect corrupted block, will be go to panic.
> 
> Because our team policy is found device DM-Verity error, device will go 
> panic.
> And then analyze what kind of device fault (crash UFS, IO error, DRAM 
> bit flip etc)
> 
> In addition to the smart phone, I would like to have an option that 
> users or administrators can use accordingly.
> There are patch contents in the attachment. I would really appreciate it 
> if you could check it.
> 
> I will look forward to hearing from yours.
> Thank you :)
> 

I do not accept that panicing the system because of verity failure is
reasonable.

In fact, even rebooting (via DM_VERITY_MODE_RESTART) looks very wrong.

The device should be put in a failed state and left for admin recovery.

Mike

Re: [dm-devel] New mode DM-Verity error handling

[Sami Tolvanen]

On Thu, Jun 18, 2020 at 11:44:45AM -0400, Mike Snitzer wrote:
> I do not accept that panicing the system because of verity failure is
> reasonable.
> 
> In fact, even rebooting (via DM_VERITY_MODE_RESTART) looks very wrong.
> 
> The device should be put in a failed state and left for admin recovery.

That's exactly how the restart mode works on some Android devices. The
bootloader sees the verification error and puts the device in recovery
mode. Using the restart mode on systems without firmware support won't
make sense, obviously.

Sami

Re: New mode DM-Verity error handling

[Mike Snitzer]

On Thu, Jun 18 2020 at 12:50pm -0400,
Sami Tolvanen <samitolvanen@google.com> wrote:

> On Thu, Jun 18, 2020 at 11:44:45AM -0400, Mike Snitzer wrote:
> > I do not accept that panicing the system because of verity failure is
> > reasonable.
> > 
> > In fact, even rebooting (via DM_VERITY_MODE_RESTART) looks very wrong.
> > 
> > The device should be put in a failed state and left for admin recovery.
> 
> That's exactly how the restart mode works on some Android devices. The
> bootloader sees the verification error and puts the device in recovery
> mode. Using the restart mode on systems without firmware support won't
> make sense, obviously.

OK, so I need further justification from Samsung why they are asking for
this panic mode.

Thanks,
Mike

Re: New mode DM-Verity error handling

[JeongHyeon Lee]

Hello Dear DM-Verity maintainers.
Thank you for your reply.

I agreed with you that "the device should be put in a failed state and 
left for admin recovery"
As dear Sami told us, When Android device occurred panic, restarting and 
to save the logs to bootloader also recovery log.
Of course Using the restart mode on systems without firmware support 
won't make sense.
However, on Android devices, restart or panic mode makes sense.

In android, the behavior is different depend on the binary type.
here are 3 type like user / userdebug / eng (engineering).

When kernel panic occurs, it operates as follows
  * kernel panic in user binary(low)-> restart mode
  * kernel panic in eng binary(mid) -> upload mode

It's actually at the debug level.
All users are set to low, but change it build option or others.
but Most users do not know.

You might think, "Why do you need a panic instead of reboot?"

To start with, It's easy to analyze what the device has problem.
If we use restart mode, it's difficult to analyze because device is 
rebooted without logging.(not remain log)
And If use panic mode, samsung takes snapshots(save log etc) when 
occurred panic.(Maybe other company or Android are same).
So We look for a debugging log and the analyze kind of problem in device 
as well as dm-verity.
In the development stage, most of them are use in eng mode.
when panic occurs, it goes to upload mode, so it is convenient to 
analyze whether it is HW problem / SW problem.
In most cases it was a hardware issue. Since we are a manufacturer, the 
HW problem is also important.

Also, users using Android devices can recognize that there is a problem 
with my device through a reboot.
Users don't know the exact reason, but they think that rebooting is wrong.
As mentioned above, in user mode, panic operates in reboot mode.
The user sees that device is rebooting and thinks there is a problem.
They uploads QnA to Samsung members App or visit service center for repair.
Then, developers need to get the log the device used by users to check 
what the problem is. So We are using panic to get the log.

What's more, reboot/panic may seem wrong, but from a security 
perspective, I think it's really important when looking at dm-verity.
Of course, I think the maintainers already know it.
To the important partition or Android devices system, will be protected 
using dm-verity.
We can make the partition(want to protect) into a read-only partition, 
compare the digest, and check whether there are any problems.
If a malicious user or hacker can damage the system or important 
partition may change something.
At this time, we can defend against further hacking by generating a 
panic or restart.
This will make the security feel strong. So reboot mode and panic mode 
will be required.

We have long explained why we need it.
Through this, Samsung needs a panic mode, so please read it carefully 
and give feedback.

Thank you :D
Jeonghyeon Lee


On 19/06/2020 02:09, Mike Snitzer wrote:
> On Thu, Jun 18 2020 at 12:50pm -0400,
> Sami Tolvanen <samitolvanen@google.com> wrote:
>
>> On Thu, Jun 18, 2020 at 11:44:45AM -0400, Mike Snitzer wrote:
>>> I do not accept that panicing the system because of verity failure is
>>> reasonable.
>>>
>>> In fact, even rebooting (via DM_VERITY_MODE_RESTART) looks very wrong.
>>>
>>> The device should be put in a failed state and left for admin recovery.
>> That's exactly how the restart mode works on some Android devices. The
>> bootloader sees the verification error and puts the device in recovery
>> mode. Using the restart mode on systems without firmware support won't
>> make sense, obviously.
> OK, so I need further justification from Samsung why they are asking for
> this panic mode.
>
> Thanks,
> Mike
>
>
>

Re: New mode DM-Verity error handling

[Milan Broz]

On 18/06/2020 19:09, Mike Snitzer wrote:
> On Thu, Jun 18 2020 at 12:50pm -0400,
> Sami Tolvanen <samitolvanen@google.com> wrote:
> 
>> On Thu, Jun 18, 2020 at 11:44:45AM -0400, Mike Snitzer wrote:
>>> I do not accept that panicing the system because of verity failure is
>>> reasonable.
>>>
>>> In fact, even rebooting (via DM_VERITY_MODE_RESTART) looks very wrong.
>>>
>>> The device should be put in a failed state and left for admin recovery.
>>
>> That's exactly how the restart mode works on some Android devices. The
>> bootloader sees the verification error and puts the device in recovery
>> mode. Using the restart mode on systems without firmware support won't
>> make sense, obviously.
> 
> OK, so I need further justification from Samsung why they are asking for
> this panic mode.

I think when we have reboot already, panic is not much better :-)

Just please note that dm-verity is used not only in Android world (with own tooling)
but in normal Linux distributions, and I need to modify userspace (veritysetup) to support
and recognize this flag.

Please *always* increase minor dm-verity target version when adding a new feature
- we can then provide some better hint if it is not supported.

Thanks,
Milan

Re: New mode DM-Verity error handling

[JeongHyeon Lee]

Dear Milan Broz.

Thank for your reply.


I didn't understand well, could you explain it in more detail?

For what reason isn't panic better?

Is it because there is a place to use other device-mapper?

Or other things? I just wonder. I would like to hear various 
explanations and information.


I just wanted user to use what they wanted through the options(flags).

Yes, If adding a new feature, modify user-space to support.


Oh, I'm sorry :(

If when i suggested new patch, i will send you a patch that increased 
minor version.

Thank you for all your detailed information.


Thanks.

JeongHyeon Lee



On 22/06/2020 16:58, Milan Broz wrote:
> On 18/06/2020 19:09, Mike Snitzer wrote:
>> On Thu, Jun 18 2020 at 12:50pm -0400,
>> Sami Tolvanen <samitolvanen@google.com> wrote:
>>
>>> On Thu, Jun 18, 2020 at 11:44:45AM -0400, Mike Snitzer wrote:
>>>> I do not accept that panicing the system because of verity failure is
>>>> reasonable.
>>>>
>>>> In fact, even rebooting (via DM_VERITY_MODE_RESTART) looks very wrong.
>>>>
>>>> The device should be put in a failed state and left for admin recovery.
>>> That's exactly how the restart mode works on some Android devices. The
>>> bootloader sees the verification error and puts the device in recovery
>>> mode. Using the restart mode on systems without firmware support won't
>>> make sense, obviously.
>> OK, so I need further justification from Samsung why they are asking for
>> this panic mode.
> I think when we have reboot already, panic is not much better :-)
>
> Just please note that dm-verity is used not only in Android world (with own tooling)
> but in normal Linux distributions, and I need to modify userspace (veritysetup) to support
> and recognize this flag.
>
> Please *always* increase minor dm-verity target version when adding a new feature
> - we can then provide some better hint if it is not supported.
>
> Thanks,
> Milan
>
>

Re: New mode DM-Verity error handling

[Milan Broz]

On 23/06/2020 01:53, JeongHyeon Lee wrote:
> 
> For what reason isn't panic better?

I did not say panic is better, I said that while we have restart already in mainline dm-verity code,
panic() is almost the same, so I see no problem in merging this patch.

Stopping system this way could create more damage if it is not configured properly,
but I think it is quite common to stop the system as fast as possible if data system integrity
is violated...

> If when i suggested new patch, i will send you a patch that increased 
> minor version.

I think Mike can fold-in version increase, if the patch is accepted.

But please include these version changes with every new feature.

Actually I am tracking it here for dm-verity as part of veritysetup userspace documentation:
  https://gitlab.com/cryptsetup/cryptsetup/-/wikis/DMVerity

Thanks,
Milan

> On 22/06/2020 16:58, Milan Broz wrote:
>> On 18/06/2020 19:09, Mike Snitzer wrote:
>>> On Thu, Jun 18 2020 at 12:50pm -0400,
>>> Sami Tolvanen <samitolvanen@google.com> wrote:
>>>
>>>> On Thu, Jun 18, 2020 at 11:44:45AM -0400, Mike Snitzer wrote:
>>>>> I do not accept that panicing the system because of verity failure is
>>>>> reasonable.
>>>>>
>>>>> In fact, even rebooting (via DM_VERITY_MODE_RESTART) looks very wrong.
>>>>>
>>>>> The device should be put in a failed state and left for admin recovery.
>>>> That's exactly how the restart mode works on some Android devices. The
>>>> bootloader sees the verification error and puts the device in recovery
>>>> mode. Using the restart mode on systems without firmware support won't
>>>> make sense, obviously.
>>> OK, so I need further justification from Samsung why they are asking for
>>> this panic mode.
>> I think when we have reboot already, panic is not much better :-)
>>
>> Just please note that dm-verity is used not only in Android world (with own tooling)
>> but in normal Linux distributions, and I need to modify userspace (veritysetup) to support
>> and recognize this flag.
>>
>> Please *always* increase minor dm-verity target version when adding a new feature
>> - we can then provide some better hint if it is not supported.
>>
>> Thanks,
>> Milan
>>
>>

Re: New mode DM-Verity error handling

[JeongHyeon Lee]

Dear Milan Broz.


Thank you for answer my query.

I asked you again because i was confused.


Yes, I also looked at the document and get a lot of information or 
studies related to dm-verity.

https://gitlab.com/cryptsetup/cryptsetup/-/wikis/DMVerity


Thank you : D

JeongHyeon Lee


On 23/06/2020 16:28, Milan Broz wrote:
> On 23/06/2020 01:53, JeongHyeon Lee wrote:
>> For what reason isn't panic better?
> I did not say panic is better, I said that while we have restart already in mainline dm-verity code,
> panic() is almost the same, so I see no problem in merging this patch.
>
> Stopping system this way could create more damage if it is not configured properly,
> but I think it is quite common to stop the system as fast as possible if data system integrity
> is violated...
>
>> If when i suggested new patch, i will send you a patch that increased
>> minor version.
> I think Mike can fold-in version increase, if the patch is accepted.
>
> But please include these version changes with every new feature.
>
> Actually I am tracking it here for dm-verity as part of veritysetup userspace documentation:
>    https://gitlab.com/cryptsetup/cryptsetup/-/wikis/DMVerity
>
> Thanks,
> Milan
>
>> On 22/06/2020 16:58, Milan Broz wrote:
>>> On 18/06/2020 19:09, Mike Snitzer wrote:
>>>> On Thu, Jun 18 2020 at 12:50pm -0400,
>>>> Sami Tolvanen <samitolvanen@google.com> wrote:
>>>>
>>>>> On Thu, Jun 18, 2020 at 11:44:45AM -0400, Mike Snitzer wrote:
>>>>>> I do not accept that panicing the system because of verity failure is
>>>>>> reasonable.
>>>>>>
>>>>>> In fact, even rebooting (via DM_VERITY_MODE_RESTART) looks very wrong.
>>>>>>
>>>>>> The device should be put in a failed state and left for admin recovery.
>>>>> That's exactly how the restart mode works on some Android devices. The
>>>>> bootloader sees the verification error and puts the device in recovery
>>>>> mode. Using the restart mode on systems without firmware support won't
>>>>> make sense, obviously.
>>>> OK, so I need further justification from Samsung why they are asking for
>>>> this panic mode.
>>> I think when we have reboot already, panic is not much better :-)
>>>
>>> Just please note that dm-verity is used not only in Android world (with own tooling)
>>> but in normal Linux distributions, and I need to modify userspace (veritysetup) to support
>>> and recognize this flag.
>>>
>>> Please *always* increase minor dm-verity target version when adding a new feature
>>> - we can then provide some better hint if it is not supported.
>>>
>>> Thanks,
>>> Milan
>>>
>>>
>