Re: [f2fs-dev] [RFC PATCH 00/10] fs-verity: filesystem-level integrity protection
Hi Jan, On Fri, Aug 31, 2018 at 10:05:23PM +0200, Jan Lübbe wrote: > On Fri, 2018-08-24 at 09:16 -0700, Eric Biggers wrote: > [...] > > Since fs-verity provides the Merkle tree root hash in constant time and > > verifies data blocks on-demand, it is useful for efficiently verifying > > the authenticity of, or "appraising", large files of which only a small > > portion may be accessed -- such as Android application (APK) files. It > > can also be useful in "audit" use cases where file hashes are logged. > > fs-verity also provides better protection against malicious disk > > firmware than an ahead-of-time hash, since fs-verity re-verifies data > > each time it's paged in. > [...] > > Feedback on the design and implementation is greatly appreciated. > > Hi, > > I've looked at the series and the slides linked form the recent lwn.net > article, but I'm not sure how fs-verity intends to protect against > malicious firmware (or offline modification). Similar to IMA/EVM, fs- > verity doesn't seem to include the name/location of the file into it's > verification. So the firmware/an attacker could replace one fs-verity- > protected file with another (maybe a trusted system APK with another > one for which a vulnerability was discovered, or /sbin/init with > /bin/sh). > > Is the expected root hash of the file provided from somewhere else, so > this is not a problem on Android? Or is this problem out-of-scope for > fs-verity? > > For IMA/EVM, there were patches by Dmitry to address this class of > attacks (they were not merged, though): > https://lwn.net/Articles/574221/ > > Thanks, > Jan > > [1] https://events.linuxfoundation.org/wp-content/uploads/2017/11/fs-ve > rify_Mike-Halcrow_Eric-Biggers.pdf Well, it's up to the user of fs-verity. If you know that, say, /bin/sh is supposed to have a fs-verity file measurement of sha256:01ba4719c80b6fe911b091a7c05124b64eeece964e09c058ef8f9805daca546b, then you can just check that. Or, after IMA support is added, users will be able to configure the fs-verity measurements to go into the IMA measurement log and/or audit log, just regular file hashes. Those records include both the file path and hash. On the other hand, if the policy you want to enforce is just that a particular file is using fs-verity and that its hash has been signed by a particular key, then indeed, if there are multiple hashes that were signed with that key, an attacker can replace the contents with a different signed contents. But that's not really fs-verity's fault; it's really the type of policy that the user chose to use on top of it, as part of their overall system security architecture. Yes, the initial plan for Android APK verification is to just use that type of policy, probably using the in-kernel signature verification support included in patch 07/10. So it will therefore have that limitation. Still, it will be a massive improvement over the status quo where attackers can make arbitrary changes to APK files post-installation, e.g. to persistently inject arbitrary malware. It will also be possible to improve the security properties of APK verification in the future by doing additional checks, such as optionally including additional file metadata in the fs-verity measurement (fs-verity's design allows for this; they can be added as authenticated extensions), or even simply checking for the correct metadata from trusted userspace code running in the /system partition. Or perhaps the trusted userspace code could download the expected fs-verity measurement of the APK from the app store given the app name. There are lots of options. But we have to start somewhere, and fs-verity is a tool that seems to be needed in any case. - Eric -- Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot ___ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [RFC PATCH 00/10] fs-verity: filesystem-level integrity protection
On Fri, 2018-08-24 at 09:16 -0700, Eric Biggers wrote: [...] > Since fs-verity provides the Merkle tree root hash in constant time and > verifies data blocks on-demand, it is useful for efficiently verifying > the authenticity of, or "appraising", large files of which only a small > portion may be accessed -- such as Android application (APK) files. It > can also be useful in "audit" use cases where file hashes are logged. > fs-verity also provides better protection against malicious disk > firmware than an ahead-of-time hash, since fs-verity re-verifies data > each time it's paged in. [...] > Feedback on the design and implementation is greatly appreciated. Hi, I've looked at the series and the slides linked form the recent lwn.net article, but I'm not sure how fs-verity intends to protect against malicious firmware (or offline modification). Similar to IMA/EVM, fs- verity doesn't seem to include the name/location of the file into it's verification. So the firmware/an attacker could replace one fs-verity- protected file with another (maybe a trusted system APK with another one for which a vulnerability was discovered, or /sbin/init with /bin/sh). Is the expected root hash of the file provided from somewhere else, so this is not a problem on Android? Or is this problem out-of-scope for fs-verity? For IMA/EVM, there were patches by Dmitry to address this class of attacks (they were not merged, though): https://lwn.net/Articles/574221/ Thanks, Jan [1] https://events.linuxfoundation.org/wp-content/uploads/2017/11/fs-ve rify_Mike-Halcrow_Eric-Biggers.pdf -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- | -- Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot ___ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
[f2fs-dev] [PATCH v2 4/5] jfs: cache NULL when both default_acl and acl are NULL
default_acl and acl of newly created inode will be initiated
as ACL_NOT_CACHED in vfs function inode_init_always() and later
will be updated by calling xxx_init_acl() in specific filesystems.
Howerver, when default_acl and acl are NULL then they keep the value
of ACL_NOT_CACHED, this patch tries to cache NULL for acl/default_acl
in this case.
Signed-off-by: Chengguang Xu
---
v1->v2:
- Coding style change.
fs/jfs/acl.c | 4
1 file changed, 4 insertions(+)
diff --git a/fs/jfs/acl.c b/fs/jfs/acl.c
index 2e71b6e7e646..8c06a6ea862d 100644
--- a/fs/jfs/acl.c
+++ b/fs/jfs/acl.c
@@ -146,12 +146,16 @@ int jfs_init_acl(tid_t tid, struct inode *inode, struct
inode *dir)
if (default_acl) {
rc = __jfs_set_acl(tid, inode, ACL_TYPE_DEFAULT, default_acl);
posix_acl_release(default_acl);
+ } else {
+ inode->i_default_acl = NULL;
}
if (acl) {
if (!rc)
rc = __jfs_set_acl(tid, inode, ACL_TYPE_ACCESS, acl);
posix_acl_release(acl);
+ } else {
+ inode->i_acl = NULL;
}
JFS_IP(inode)->mode2 = (JFS_IP(inode)->mode2 & 0x) |
--
2.17.1
--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
[f2fs-dev] [PATCH v2 5/5] orangefs: cache NULL when both default_acl and acl are NULL
default_acl and acl of newly created inode will be initiated
as ACL_NOT_CACHED in vfs function inode_init_always() and later
will be updated by calling xxx_init_acl() in specific filesystems.
Howerver, when default_acl and acl are NULL then they keep the value
of ACL_NOT_CACHED, this patch tries to cache NULL for acl/default_acl
in this case.
Signed-off-by: Chengguang Xu
---
v1->v2:
- Coding style change.
fs/orangefs/acl.c | 4
1 file changed, 4 insertions(+)
diff --git a/fs/orangefs/acl.c b/fs/orangefs/acl.c
index 10587413b20e..72d2ff17d27b 100644
--- a/fs/orangefs/acl.c
+++ b/fs/orangefs/acl.c
@@ -167,12 +167,16 @@ int orangefs_init_acl(struct inode *inode, struct inode
*dir)
error = __orangefs_set_acl(inode, default_acl,
ACL_TYPE_DEFAULT);
posix_acl_release(default_acl);
+ } else {
+ inode->i_default_acl = NULL;
}
if (acl) {
if (!error)
error = __orangefs_set_acl(inode, acl, ACL_TYPE_ACCESS);
posix_acl_release(acl);
+ } else {
+ inode->i_acl = NULL;
}
/* If mode of the inode was changed, then do a forcible ->setattr */
--
2.17.1
--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
[f2fs-dev] [PATCH v2 2/5] ext4: cache NULL when both default_acl and acl are NULL
default_acl and acl of newly created inode will be initiated
as ACL_NOT_CACHED in vfs function inode_init_always() and later
will be updated by calling xxx_init_acl() in specific filesystems.
Howerver, when default_acl and acl are NULL then they keep the value
of ACL_NOT_CACHED, this patch tries to cache NULL for acl/default_acl
in this case.
Signed-off-by: Chengguang Xu
---
v1->v2:
- Coding style change.
fs/ext4/acl.c | 4
1 file changed, 4 insertions(+)
diff --git a/fs/ext4/acl.c b/fs/ext4/acl.c
index fb50f9aa6ead..c1d570ee1d9f 100644
--- a/fs/ext4/acl.c
+++ b/fs/ext4/acl.c
@@ -284,12 +284,16 @@ ext4_init_acl(handle_t *handle, struct inode *inode,
struct inode *dir)
error = __ext4_set_acl(handle, inode, ACL_TYPE_DEFAULT,
default_acl, XATTR_CREATE);
posix_acl_release(default_acl);
+ } else {
+ inode->i_default_acl = NULL;
}
if (acl) {
if (!error)
error = __ext4_set_acl(handle, inode, ACL_TYPE_ACCESS,
acl, XATTR_CREATE);
posix_acl_release(acl);
+ } else {
+ inode->i_acl = NULL;
}
return error;
}
--
2.17.1
--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
[f2fs-dev] [PATCH v2 1/5] ext2: cache NULL when both default_acl and acl are NULL
default_acl and acl of newly created inode will be initiated
as ACL_NOT_CACHED in vfs function inode_init_always() and later
will be updated by calling xxx_init_acl() in specific filesystems.
Howerver, when default_acl and acl are NULL then they keep the value
of ACL_NOT_CACHED, this patch tries to cache NULL for acl/default_acl
in this case.
Signed-off-by: Chengguang Xu
---
v1->v2:
- Coding style change.
fs/ext2/acl.c | 4
1 file changed, 4 insertions(+)
diff --git a/fs/ext2/acl.c b/fs/ext2/acl.c
index 224c04abb2e5..cf4c77f8dd08 100644
--- a/fs/ext2/acl.c
+++ b/fs/ext2/acl.c
@@ -256,11 +256,15 @@ ext2_init_acl(struct inode *inode, struct inode *dir)
if (default_acl) {
error = __ext2_set_acl(inode, default_acl, ACL_TYPE_DEFAULT);
posix_acl_release(default_acl);
+ } else {
+ inode->i_default_acl = NULL;
}
if (acl) {
if (!error)
error = __ext2_set_acl(inode, acl, ACL_TYPE_ACCESS);
posix_acl_release(acl);
+ } else {
+ inode->i_acl = NULL;
}
return error;
}
--
2.17.1
--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH] f2fs: add additional sanity check in f2fs_acl_from_disk()
On 2018/8/30 21:33, Chengguang Xu wrote: > Add additinal sanity check for irregular case(e.g. corruption). > If size of extended attribution is smaller than size of acl header, > then return -EINVAL. > > Signed-off-by: Chengguang Xu Reviewed-by: Chao Yu Thanks, -- Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot ___ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH] f2fs: add additional sanity check in f2fs_acl_from_disk()
On 2018/8/31 19:40, Chengguang Xu wrote:
>
>
> On 2018/8/31 at 下午3:02, Chao Yu wrote:
>
>> On 2018/8/31 0:19, cgxu519 wrote:
>>>
>>> On 08/30/2018 11:41 PM, Chao Yu wrote:
Hi Chengguang,
On 2018/8/30 21:33, Chengguang Xu wrote:
> Add additinal sanity check for irregular case(e.g. corruption).
> If size of extended attribution is smaller than size of acl header,
> then return -EINVAL.
>
> Signed-off-by: Chengguang Xu
> ---
> fs/f2fs/acl.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/fs/f2fs/acl.c b/fs/f2fs/acl.c
> index 111824199a88..79e9ea773070 100644
> --- a/fs/f2fs/acl.c
> +++ b/fs/f2fs/acl.c
> @@ -53,6 +53,9 @@ static struct posix_acl *f2fs_acl_from_disk(const char
> *value, size_t size)
> struct f2fs_acl_entry *entry = (struct f2fs_acl_entry *)(hdr +
> 1);
> const char *end = value + size;
>
> + if (size < sizeof(f2fs_acl_header))
> + return ERR_PTR(-EINVAL);
I guess below codes have checked that already?
count = f2fs_acl_count(size);
if (count < 0)
return ERR_PTR(-EINVAL);
>>>
>>> Hi Chao,
>>>
>>> Thanks for prompt reply.
>>>
>>> I still think in a rare case, it can pass the check in f2fs_acl_count()
>>> and cause unexpected behavior.
>>>
>>> For example, like below code path in f2fs_acl_count().
>>
>> if size < sizeof(f2fs_acl_header)
>>
>> size -= sizeof(struct f2fs_acl_header);
>>
>> size should be smaller than zero, right?
>>
>>>
>>> -> if (s < 0) {
>>> if (size % sizeof(struct f2fs_acl_entry_short))
>>> return -1;
>>> -> return size / sizeof(struct f2fs_acl_entry_short);
>>
>> So the return value should be smaller than zero?
>
> size is unsigned so the return value will not be negative here.
You're right, I misread size_t as ssize_t, sorry.
Thanks,
>
> Thanks,
> Chengguang
>
> .
>
--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH] f2fs: add additional sanity check in f2fs_acl_from_disk()
On 2018/8/31 at 下午3:02, Chao Yu wrote:
> On 2018/8/31 0:19, cgxu519 wrote:
> >
> > On 08/30/2018 11:41 PM, Chao Yu wrote:
> >> Hi Chengguang,
> >>
> >> On 2018/8/30 21:33, Chengguang Xu wrote:
> >>> Add additinal sanity check for irregular case(e.g. corruption).
> >>> If size of extended attribution is smaller than size of acl header,
> >>> then return -EINVAL.
> >>>
> >>> Signed-off-by: Chengguang Xu
> >>> ---
> >>> fs/f2fs/acl.c | 3 +++
> >>> 1 file changed, 3 insertions(+)
> >>>
> >>> diff --git a/fs/f2fs/acl.c b/fs/f2fs/acl.c
> >>> index 111824199a88..79e9ea773070 100644
> >>> --- a/fs/f2fs/acl.c
> >>> +++ b/fs/f2fs/acl.c
> >>> @@ -53,6 +53,9 @@ static struct posix_acl *f2fs_acl_from_disk(const char
> >>> *value, size_t size)
> >>> struct f2fs_acl_entry *entry = (struct f2fs_acl_entry *)(hdr +
> >>> 1);
> >>> const char *end = value + size;
> >>>
> >>> + if (size < sizeof(f2fs_acl_header))
> >>> + return ERR_PTR(-EINVAL);
> >> I guess below codes have checked that already?
> >>
> >>count = f2fs_acl_count(size);
> >>if (count < 0)
> >>return ERR_PTR(-EINVAL);
> >
> > Hi Chao,
> >
> > Thanks for prompt reply.
> >
> > I still think in a rare case, it can pass the check in f2fs_acl_count()
> > and cause unexpected behavior.
> >
> > For example, like below code path in f2fs_acl_count().
>
> if size < sizeof(f2fs_acl_header)
>
> size -= sizeof(struct f2fs_acl_header);
>
> size should be smaller than zero, right?
>
> >
> > -> if (s < 0) {
> > if (size % sizeof(struct f2fs_acl_entry_short))
> > return -1;
> > -> return size / sizeof(struct f2fs_acl_entry_short);
>
> So the return value should be smaller than zero?
size is unsigned so the return value will not be negative here.
Thanks,
Chengguang
--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
[f2fs-dev] [PATCH] f2fs: fix unnecessary periodic wakeup of discard thread when dev is busy
When dev is busy, discard thread wake up timeout can be aligned with the
exact time that it needs to wait for dev to come out of busy. This helps
to avoid unnecessary periodic wakeups and thus save some power.
Signed-off-by: Sahitya Tummala
---
fs/f2fs/segment.c | 8 +++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
index 8bcbb50..df14030 100644
--- a/fs/f2fs/segment.c
+++ b/fs/f2fs/segment.c
@@ -1379,6 +1379,8 @@ static int issue_discard_thread(void *data)
struct discard_policy dpolicy;
unsigned int wait_ms = DEF_MIN_DISCARD_ISSUE_TIME;
int issued;
+ unsigned long interval = sbi->interval_time[REQ_TIME] * HZ;
+ long delta;
set_freezable();
@@ -1410,7 +1412,11 @@ static int issue_discard_thread(void *data)
__wait_all_discard_cmd(sbi, );
wait_ms = dpolicy.min_interval;
} else if (issued == -1){
- wait_ms = dpolicy.mid_interval;
+ delta = (sbi->last_time[REQ_TIME] + interval) - jiffies;
+ if (delta > 0)
+ wait_ms = jiffies_to_msecs(delta);
+ else
+ wait_ms = dpolicy.mid_interval;
} else {
wait_ms = dpolicy.max_interval;
}
--
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux
Foundation Collaborative Project.
--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
[f2fs-dev] [Bug 200951] kernel NULL pointer dereference in update_sit_entry
https://bugzilla.kernel.org/show_bug.cgi?id=200951 Chao Yu (c...@kernel.org) changed: What|Removed |Added CC||c...@kernel.org --- Comment #3 from Chao Yu (c...@kernel.org) --- Hi, Is this bug reproducible? Can you run fsck on device to check filesystem consistence. -- You are receiving this mail because: You are watching the assignee of the bug. -- Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot ___ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
[f2fs-dev] [Bug 200871] F2FS experiences data loss (entry is completely lost) when an I/O failure occurs.
https://bugzilla.kernel.org/show_bug.cgi?id=200871 --- Comment #3 from Chao Yu (c...@kernel.org) --- Alright, can you please provider the complete testcase including error injection script for me? I'd like to reproduce in my environment. -- You are receiving this mail because: You are watching the assignee of the bug. -- Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot ___ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH] f2fs: add additional sanity check in f2fs_acl_from_disk()
On 2018/8/31 0:19, cgxu519 wrote:
>
> On 08/30/2018 11:41 PM, Chao Yu wrote:
>> Hi Chengguang,
>>
>> On 2018/8/30 21:33, Chengguang Xu wrote:
>>> Add additinal sanity check for irregular case(e.g. corruption).
>>> If size of extended attribution is smaller than size of acl header,
>>> then return -EINVAL.
>>>
>>> Signed-off-by: Chengguang Xu
>>> ---
>>> fs/f2fs/acl.c | 3 +++
>>> 1 file changed, 3 insertions(+)
>>>
>>> diff --git a/fs/f2fs/acl.c b/fs/f2fs/acl.c
>>> index 111824199a88..79e9ea773070 100644
>>> --- a/fs/f2fs/acl.c
>>> +++ b/fs/f2fs/acl.c
>>> @@ -53,6 +53,9 @@ static struct posix_acl *f2fs_acl_from_disk(const char
>>> *value, size_t size)
>>> struct f2fs_acl_entry *entry = (struct f2fs_acl_entry *)(hdr + 1);
>>> const char *end = value + size;
>>>
>>> + if (size < sizeof(f2fs_acl_header))
>>> + return ERR_PTR(-EINVAL);
>> I guess below codes have checked that already?
>>
>> count = f2fs_acl_count(size);
>> if (count < 0)
>> return ERR_PTR(-EINVAL);
>
> Hi Chao,
>
> Thanks for prompt reply.
>
> I still think in a rare case, it can pass the check in f2fs_acl_count()
> and cause unexpected behavior.
>
> For example, like below code path in f2fs_acl_count().
if size < sizeof(f2fs_acl_header)
size -= sizeof(struct f2fs_acl_header);
size should be smaller than zero, right?
>
> -> if (s < 0) {
> if (size % sizeof(struct f2fs_acl_entry_short))
> return -1;
> -> return size / sizeof(struct f2fs_acl_entry_short);
So the return value should be smaller than zero?
Thanks,
> }
>
>
> Thanks,
> Chengguang
>
>
>
>
>
>
>
>
>
>
>
>
> .
>
--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
