| Message ID | 20240130214620.3155380-5-stefanb@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | evm: Support signatures on stacked filesystem | expand |
On 1/30/24 16:46, Stefan Berger wrote: > Changes to the file attribute (mode bits, uid, gid) on the lower layer > are not take into account when d_backing_inode() is used when a file is > accessed on the overlay layer and this file has not yet been copied up. > This is because d_backing_inode() does not return the real inode of the > lower layer but instead returns the backing inode which holds old file > attributes. When the old file attributes are used for calculating the > metadata hash then the expected hash is calculated and the file then > mistakenly passes signature verification. Therefore, use d_real_inode() > which returns the inode of the lower layer for as long as the file has > not been copied up and returns the upper layer's inode otherwise. > > Signed-off-by: Stefan Berger <stefanb@linux.ibm.com> > --- > security/integrity/evm/evm_crypto.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/security/integrity/evm/evm_crypto.c b/security/integrity/evm/evm_crypto.c > index b1ffd4cc0b44..2e48fe54e899 100644 > --- a/security/integrity/evm/evm_crypto.c > +++ b/security/integrity/evm/evm_crypto.c > @@ -223,7 +223,7 @@ static int evm_calc_hmac_or_hash(struct dentry *dentry, > size_t req_xattr_value_len, > uint8_t type, struct evm_digest *data) > { > - struct inode *inode = d_backing_inode(dentry); > + struct inode *inode = d_real_inode(dentry); > struct xattr_list *xattr; > struct shash_desc *desc; > size_t xattr_size = 0; We need this patch when NOT activating CONFIG_OVERLAY_FS_METACOPY but when setting CONFIG_OVERLAY_FS_METACOPY=y it has to be reverted... I am not sure what the solution is.
On Wed, Jan 31, 2024 at 4:11 AM Stefan Berger <stefanb@linux.ibm.com> wrote: > > > > On 1/30/24 16:46, Stefan Berger wrote: > > Changes to the file attribute (mode bits, uid, gid) on the lower layer > > are not take into account when d_backing_inode() is used when a file is > > accessed on the overlay layer and this file has not yet been copied up. > > This is because d_backing_inode() does not return the real inode of the > > lower layer but instead returns the backing inode which holds old file > > attributes. When the old file attributes are used for calculating the > > metadata hash then the expected hash is calculated and the file then > > mistakenly passes signature verification. Therefore, use d_real_inode() > > which returns the inode of the lower layer for as long as the file has > > not been copied up and returns the upper layer's inode otherwise. > > > > Signed-off-by: Stefan Berger <stefanb@linux.ibm.com> > > --- > > security/integrity/evm/evm_crypto.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/security/integrity/evm/evm_crypto.c b/security/integrity/evm/evm_crypto.c > > index b1ffd4cc0b44..2e48fe54e899 100644 > > --- a/security/integrity/evm/evm_crypto.c > > +++ b/security/integrity/evm/evm_crypto.c > > @@ -223,7 +223,7 @@ static int evm_calc_hmac_or_hash(struct dentry *dentry, > > size_t req_xattr_value_len, > > uint8_t type, struct evm_digest *data) > > { > > - struct inode *inode = d_backing_inode(dentry); > > + struct inode *inode = d_real_inode(dentry); > > struct xattr_list *xattr; > > struct shash_desc *desc; > > size_t xattr_size = 0; > > We need this patch when NOT activating CONFIG_OVERLAY_FS_METACOPY but > when setting CONFIG_OVERLAY_FS_METACOPY=y it has to be reverted... I am > not sure what the solution is. I think d_real_inode() does not work correctly for all its current users for a metacopy file. I think the solution is to change d_real_inode() to return the data inode and add another helper to get the metadata inode if needed. I will post some patches for it. However, I must say that I do not know if evm_calc_hmac_or_hash() needs the lower data inode, the upper metadata inode or both. The last time you tried to fix ovl+IMA, I asked for documentation of what data/metadata is protected with EVM and how are those protections supposed to work across overlayfs copy up, when the data and metadata are often split between 2 and myabe event 3 differnt inode. From the current patch set, I still don't understand what is the expected behavior before and after copy up of data/metadata-only. Thanks, Amir.
On 1/31/24 08:16, Amir Goldstein wrote: > On Wed, Jan 31, 2024 at 4:11 AM Stefan Berger <stefanb@linux.ibm.com> wrote: >> >> >> >> On 1/30/24 16:46, Stefan Berger wrote: >>> Changes to the file attribute (mode bits, uid, gid) on the lower layer >>> are not take into account when d_backing_inode() is used when a file is >>> accessed on the overlay layer and this file has not yet been copied up. >>> This is because d_backing_inode() does not return the real inode of the >>> lower layer but instead returns the backing inode which holds old file >>> attributes. When the old file attributes are used for calculating the >>> metadata hash then the expected hash is calculated and the file then >>> mistakenly passes signature verification. Therefore, use d_real_inode() >>> which returns the inode of the lower layer for as long as the file has >>> not been copied up and returns the upper layer's inode otherwise. >>> >>> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com> >>> --- >>> security/integrity/evm/evm_crypto.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/security/integrity/evm/evm_crypto.c b/security/integrity/evm/evm_crypto.c >>> index b1ffd4cc0b44..2e48fe54e899 100644 >>> --- a/security/integrity/evm/evm_crypto.c >>> +++ b/security/integrity/evm/evm_crypto.c >>> @@ -223,7 +223,7 @@ static int evm_calc_hmac_or_hash(struct dentry *dentry, >>> size_t req_xattr_value_len, >>> uint8_t type, struct evm_digest *data) >>> { >>> - struct inode *inode = d_backing_inode(dentry); >>> + struct inode *inode = d_real_inode(dentry); >>> struct xattr_list *xattr; >>> struct shash_desc *desc; >>> size_t xattr_size = 0; >> >> We need this patch when NOT activating CONFIG_OVERLAY_FS_METACOPY but >> when setting CONFIG_OVERLAY_FS_METACOPY=y it has to be reverted... I am >> not sure what the solution is. > > I think d_real_inode() does not work correctly for all its current users for > a metacopy file. > > I think the solution is to change d_real_inode() to return the data inode > and add another helper to get the metadata inode if needed. > I will post some patches for it. I thought that we may have to go through vfs_getattr() but even better if we don't because we don't have the file *file anywhere 'near'. > > However, I must say that I do not know if evm_calc_hmac_or_hash() > needs the lower data inode, the upper metadata inode or both. What it needs are data structures with mode bits, uid, and gid that stat in userspace would show. > > The last time you tried to fix ovl+IMA, I asked for documentation > of what data/metadata is protected with EVM and how are those > protections supposed to work across overlayfs copy up, when the > data and metadata are often split between 2 and myabe event 3 > differnt inode. I always compare against what userspace sees with stat and that's what the EVM should also work with so it ends up in reasonable matching result in terms of hash calculation and then access permission/rejection. > > From the current patch set, I still don't understand what is the expected > behavior before and after copy up of data/metadata-only. > > Thanks, > Amir.
On Wed, Jan 31, 2024 at 4:40 PM Stefan Berger <stefanb@linux.ibm.com> wrote: > > > > On 1/31/24 08:16, Amir Goldstein wrote: > > On Wed, Jan 31, 2024 at 4:11 AM Stefan Berger <stefanb@linux.ibm.com> wrote: > >> > >> > >> > >> On 1/30/24 16:46, Stefan Berger wrote: > >>> Changes to the file attribute (mode bits, uid, gid) on the lower layer > >>> are not take into account when d_backing_inode() is used when a file is > >>> accessed on the overlay layer and this file has not yet been copied up. > >>> This is because d_backing_inode() does not return the real inode of the > >>> lower layer but instead returns the backing inode which holds old file > >>> attributes. When the old file attributes are used for calculating the > >>> metadata hash then the expected hash is calculated and the file then > >>> mistakenly passes signature verification. Therefore, use d_real_inode() > >>> which returns the inode of the lower layer for as long as the file has > >>> not been copied up and returns the upper layer's inode otherwise. > >>> > >>> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com> > >>> --- > >>> security/integrity/evm/evm_crypto.c | 2 +- > >>> 1 file changed, 1 insertion(+), 1 deletion(-) > >>> > >>> diff --git a/security/integrity/evm/evm_crypto.c b/security/integrity/evm/evm_crypto.c > >>> index b1ffd4cc0b44..2e48fe54e899 100644 > >>> --- a/security/integrity/evm/evm_crypto.c > >>> +++ b/security/integrity/evm/evm_crypto.c > >>> @@ -223,7 +223,7 @@ static int evm_calc_hmac_or_hash(struct dentry *dentry, > >>> size_t req_xattr_value_len, > >>> uint8_t type, struct evm_digest *data) > >>> { > >>> - struct inode *inode = d_backing_inode(dentry); > >>> + struct inode *inode = d_real_inode(dentry); > >>> struct xattr_list *xattr; > >>> struct shash_desc *desc; > >>> size_t xattr_size = 0; > >> > >> We need this patch when NOT activating CONFIG_OVERLAY_FS_METACOPY but > >> when setting CONFIG_OVERLAY_FS_METACOPY=y it has to be reverted... I am > >> not sure what the solution is. > > > > I think d_real_inode() does not work correctly for all its current users for > > a metacopy file. > > > > I think the solution is to change d_real_inode() to return the data inode > > and add another helper to get the metadata inode if needed. > > I will post some patches for it. > > I thought that we may have to go through vfs_getattr() but even better > if we don't because we don't have the file *file anywhere 'near'. > > > > > However, I must say that I do not know if evm_calc_hmac_or_hash() > > needs the lower data inode, the upper metadata inode or both. > > What it needs are data structures with mode bits, uid, and gid that stat > in userspace would show. > > With or without metacopy enabled, an overlay inode st_uid st_gid st_mode are always taken from the upper most inode which is what d_real_inode() currently returns, so I do not understand what the problem is. > > > > The last time you tried to fix ovl+IMA, I asked for documentation > > of what data/metadata is protected with EVM and how are those > > protections supposed to work across overlayfs copy up, when the > > data and metadata are often split between 2 and myabe event 3 > > differnt inode. > > I always compare against what userspace sees with stat and that's what > the EVM should also work with so it ends up in reasonable matching > result in terms of hash calculation and then access permission/rejection. > I will need a lot more analysis information to be able to help you. Exactly which setup, exactly which test, exactly which inode/dentry/file objects are used and how they are accessed when things go wrong. Thanks, Amir.
On Wed, Jan 31, 2024 at 5:54 PM Amir Goldstein <amir73il@gmail.com> wrote: > > On Wed, Jan 31, 2024 at 4:40 PM Stefan Berger <stefanb@linux.ibm.com> wrote: > > > > > > > > On 1/31/24 08:16, Amir Goldstein wrote: > > > On Wed, Jan 31, 2024 at 4:11 AM Stefan Berger <stefanb@linux.ibm.com> wrote: > > >> > > >> > > >> > > >> On 1/30/24 16:46, Stefan Berger wrote: > > >>> Changes to the file attribute (mode bits, uid, gid) on the lower layer > > >>> are not take into account when d_backing_inode() is used when a file is > > >>> accessed on the overlay layer and this file has not yet been copied up. > > >>> This is because d_backing_inode() does not return the real inode of the > > >>> lower layer but instead returns the backing inode which holds old file > > >>> attributes. When the old file attributes are used for calculating the > > >>> metadata hash then the expected hash is calculated and the file then > > >>> mistakenly passes signature verification. Therefore, use d_real_inode() > > >>> which returns the inode of the lower layer for as long as the file has > > >>> not been copied up and returns the upper layer's inode otherwise. > > >>> > > >>> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com> > > >>> --- > > >>> security/integrity/evm/evm_crypto.c | 2 +- > > >>> 1 file changed, 1 insertion(+), 1 deletion(-) > > >>> > > >>> diff --git a/security/integrity/evm/evm_crypto.c b/security/integrity/evm/evm_crypto.c > > >>> index b1ffd4cc0b44..2e48fe54e899 100644 > > >>> --- a/security/integrity/evm/evm_crypto.c > > >>> +++ b/security/integrity/evm/evm_crypto.c > > >>> @@ -223,7 +223,7 @@ static int evm_calc_hmac_or_hash(struct dentry *dentry, > > >>> size_t req_xattr_value_len, > > >>> uint8_t type, struct evm_digest *data) > > >>> { > > >>> - struct inode *inode = d_backing_inode(dentry); > > >>> + struct inode *inode = d_real_inode(dentry); > > >>> struct xattr_list *xattr; > > >>> struct shash_desc *desc; > > >>> size_t xattr_size = 0; > > >> > > >> We need this patch when NOT activating CONFIG_OVERLAY_FS_METACOPY but > > >> when setting CONFIG_OVERLAY_FS_METACOPY=y it has to be reverted... I am > > >> not sure what the solution is. > > > > > > I think d_real_inode() does not work correctly for all its current users for > > > a metacopy file. > > > > > > I think the solution is to change d_real_inode() to return the data inode > > > and add another helper to get the metadata inode if needed. > > > I will post some patches for it. > > > > I thought that we may have to go through vfs_getattr() but even better > > if we don't because we don't have the file *file anywhere 'near'. > > > > > > > > However, I must say that I do not know if evm_calc_hmac_or_hash() > > > needs the lower data inode, the upper metadata inode or both. > > > > What it needs are data structures with mode bits, uid, and gid that stat > > in userspace would show. > > > > > > With or without metacopy enabled, an overlay inode st_uid st_gid st_mode > are always taken from the upper most inode which is what d_real_inode() > currently returns, so I do not understand what the problem is. > No, I was wrong. It is the other way around. d_real_inode() always returns the real data inode and you need the upper most real inode. You can try this: - struct inode *inode = d_backing_inode(dentry); + struct inode *inode = d_inode(d_real(dentry, false)); With the changes in: https://github.com/amir73il/linux/commits/overlayfs-devel/ Not thoroughly tested... Thanks, Amir.
On 1/31/24 10:54, Amir Goldstein wrote: > On Wed, Jan 31, 2024 at 4:40 PM Stefan Berger <stefanb@linux.ibm.com> wrote: >> >> >> >> On 1/31/24 08:16, Amir Goldstein wrote: >>> On Wed, Jan 31, 2024 at 4:11 AM Stefan Berger <stefanb@linux.ibm.com> wrote: >>>> >>>> >>>> >>>> On 1/30/24 16:46, Stefan Berger wrote: >>>>> Changes to the file attribute (mode bits, uid, gid) on the lower layer >>>>> are not take into account when d_backing_inode() is used when a file is >>>>> accessed on the overlay layer and this file has not yet been copied up. >>>>> This is because d_backing_inode() does not return the real inode of the >>>>> lower layer but instead returns the backing inode which holds old file >>>>> attributes. When the old file attributes are used for calculating the >>>>> metadata hash then the expected hash is calculated and the file then >>>>> mistakenly passes signature verification. Therefore, use d_real_inode() >>>>> which returns the inode of the lower layer for as long as the file has >>>>> not been copied up and returns the upper layer's inode otherwise. >>>>> >>>>> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com> >>>>> --- >>>>> security/integrity/evm/evm_crypto.c | 2 +- >>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>> >>>>> diff --git a/security/integrity/evm/evm_crypto.c b/security/integrity/evm/evm_crypto.c >>>>> index b1ffd4cc0b44..2e48fe54e899 100644 >>>>> --- a/security/integrity/evm/evm_crypto.c >>>>> +++ b/security/integrity/evm/evm_crypto.c >>>>> @@ -223,7 +223,7 @@ static int evm_calc_hmac_or_hash(struct dentry *dentry, >>>>> size_t req_xattr_value_len, >>>>> uint8_t type, struct evm_digest *data) >>>>> { >>>>> - struct inode *inode = d_backing_inode(dentry); >>>>> + struct inode *inode = d_real_inode(dentry); >>>>> struct xattr_list *xattr; >>>>> struct shash_desc *desc; >>>>> size_t xattr_size = 0; >>>> >>>> We need this patch when NOT activating CONFIG_OVERLAY_FS_METACOPY but >>>> when setting CONFIG_OVERLAY_FS_METACOPY=y it has to be reverted... I am >>>> not sure what the solution is. >>> >>> I think d_real_inode() does not work correctly for all its current users for >>> a metacopy file. >>> >>> I think the solution is to change d_real_inode() to return the data inode >>> and add another helper to get the metadata inode if needed. >>> I will post some patches for it. >> >> I thought that we may have to go through vfs_getattr() but even better >> if we don't because we don't have the file *file anywhere 'near'. >> >>> >>> However, I must say that I do not know if evm_calc_hmac_or_hash() >>> needs the lower data inode, the upper metadata inode or both. >> >> What it needs are data structures with mode bits, uid, and gid that stat >> in userspace would show. >> >> > > With or without metacopy enabled, an overlay inode st_uid st_gid st_mode > are always taken from the upper most inode which is what d_real_inode() > currently returns, so I do not understand what the problem is. I have testcases that work fine with this series when CONFIG_OVERLAY_FS_METACOPY is not active. Once I activate this then a test case that changes a file's gid on the overlay layer from 0 to '12' while causing a copy-up allows a file to execute even thugh it should not execute. The reason is because d_real_inode(dentry)->i_guid shows the '0' while d_backing_dentry(dentry)->i_guid shows '12'. User space stat also shows '12' as expected. Just saw your other email, will try that now ...
On 1/31/24 12:23, Amir Goldstein wrote: > On Wed, Jan 31, 2024 at 5:54 PM Amir Goldstein <amir73il@gmail.com> wrote: >> >> On Wed, Jan 31, 2024 at 4:40 PM Stefan Berger <stefanb@linux.ibm.com> wrote: >>> >>> >>> >>> On 1/31/24 08:16, Amir Goldstein wrote: >>>> On Wed, Jan 31, 2024 at 4:11 AM Stefan Berger <stefanb@linux.ibm.com> wrote: >>>>> >>>>> >>>>> >>>>> On 1/30/24 16:46, Stefan Berger wrote: >>>>>> Changes to the file attribute (mode bits, uid, gid) on the lower layer >>>>>> are not take into account when d_backing_inode() is used when a file is >>>>>> accessed on the overlay layer and this file has not yet been copied up. >>>>>> This is because d_backing_inode() does not return the real inode of the >>>>>> lower layer but instead returns the backing inode which holds old file >>>>>> attributes. When the old file attributes are used for calculating the >>>>>> metadata hash then the expected hash is calculated and the file then >>>>>> mistakenly passes signature verification. Therefore, use d_real_inode() >>>>>> which returns the inode of the lower layer for as long as the file has >>>>>> not been copied up and returns the upper layer's inode otherwise. >>>>>> >>>>>> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com> >>>>>> --- >>>>>> security/integrity/evm/evm_crypto.c | 2 +- >>>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>>> >>>>>> diff --git a/security/integrity/evm/evm_crypto.c b/security/integrity/evm/evm_crypto.c >>>>>> index b1ffd4cc0b44..2e48fe54e899 100644 >>>>>> --- a/security/integrity/evm/evm_crypto.c >>>>>> +++ b/security/integrity/evm/evm_crypto.c >>>>>> @@ -223,7 +223,7 @@ static int evm_calc_hmac_or_hash(struct dentry *dentry, >>>>>> size_t req_xattr_value_len, >>>>>> uint8_t type, struct evm_digest *data) >>>>>> { >>>>>> - struct inode *inode = d_backing_inode(dentry); >>>>>> + struct inode *inode = d_real_inode(dentry); >>>>>> struct xattr_list *xattr; >>>>>> struct shash_desc *desc; >>>>>> size_t xattr_size = 0; >>>>> >>>>> We need this patch when NOT activating CONFIG_OVERLAY_FS_METACOPY but >>>>> when setting CONFIG_OVERLAY_FS_METACOPY=y it has to be reverted... I am >>>>> not sure what the solution is. >>>> >>>> I think d_real_inode() does not work correctly for all its current users for >>>> a metacopy file. >>>> >>>> I think the solution is to change d_real_inode() to return the data inode >>>> and add another helper to get the metadata inode if needed. >>>> I will post some patches for it. >>> >>> I thought that we may have to go through vfs_getattr() but even better >>> if we don't because we don't have the file *file anywhere 'near'. >>> >>>> >>>> However, I must say that I do not know if evm_calc_hmac_or_hash() >>>> needs the lower data inode, the upper metadata inode or both. >>> >>> What it needs are data structures with mode bits, uid, and gid that stat >>> in userspace would show. >>> >>> >> >> With or without metacopy enabled, an overlay inode st_uid st_gid st_mode >> are always taken from the upper most inode which is what d_real_inode() >> currently returns, so I do not understand what the problem is. >> > > No, I was wrong. It is the other way around. > d_real_inode() always returns the real data inode and you need the > upper most real inode. > > You can try this: > > - struct inode *inode = d_backing_inode(dentry); > + struct inode *inode = d_inode(d_real(dentry, false)); > > With the changes in: > > https://github.com/amir73il/linux/commits/overlayfs-devel/ > > Not thoroughly tested... The change + 3 topmost patches cherry-picked is unfortunately are crashing for me. FYI - in case you are interested: My tests running this with UML are here: repo: https://github.com/stefanberger/ima-namespaces-tests.git branch: overlayfs There's a UML config in config/config.uml Compiler kernel with this series applied: make ARCH=um -j128 && yes | cp linux /usr/local/bin/linux sudo IMA_TEST_UML=/usr/local/bin/linux IMA_TEST_VERBOSE=0 evm+overlayfs-1/test.sh sudo IMA_TEST_UML=/usr/local/bin/linux IMA_TEST_VERBOSE=0 evm+overlayfs-2/test.sh sudo IMA_TEST_UML=/usr/local/bin/linux IMA_TEST_VERBOSE=0 evm+overlayfs-3/test.sh The 2nd and 3rd test case will fail at some point when metacopy is enabled, otherwise they will all pass. > > Thanks, > Amir. >
On Wed, Jan 31, 2024 at 7:46 PM Stefan Berger <stefanb@linux.ibm.com> wrote: > > > > On 1/31/24 12:23, Amir Goldstein wrote: > > On Wed, Jan 31, 2024 at 5:54 PM Amir Goldstein <amir73il@gmail.com> wrote: > >> > >> On Wed, Jan 31, 2024 at 4:40 PM Stefan Berger <stefanb@linux.ibm.com> wrote: > >>> > >>> > >>> > >>> On 1/31/24 08:16, Amir Goldstein wrote: > >>>> On Wed, Jan 31, 2024 at 4:11 AM Stefan Berger <stefanb@linux.ibm.com> wrote: > >>>>> > >>>>> > >>>>> > >>>>> On 1/30/24 16:46, Stefan Berger wrote: > >>>>>> Changes to the file attribute (mode bits, uid, gid) on the lower layer > >>>>>> are not take into account when d_backing_inode() is used when a file is > >>>>>> accessed on the overlay layer and this file has not yet been copied up. > >>>>>> This is because d_backing_inode() does not return the real inode of the > >>>>>> lower layer but instead returns the backing inode which holds old file > >>>>>> attributes. When the old file attributes are used for calculating the > >>>>>> metadata hash then the expected hash is calculated and the file then > >>>>>> mistakenly passes signature verification. Therefore, use d_real_inode() > >>>>>> which returns the inode of the lower layer for as long as the file has > >>>>>> not been copied up and returns the upper layer's inode otherwise. > >>>>>> > >>>>>> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com> > >>>>>> --- > >>>>>> security/integrity/evm/evm_crypto.c | 2 +- > >>>>>> 1 file changed, 1 insertion(+), 1 deletion(-) > >>>>>> > >>>>>> diff --git a/security/integrity/evm/evm_crypto.c b/security/integrity/evm/evm_crypto.c > >>>>>> index b1ffd4cc0b44..2e48fe54e899 100644 > >>>>>> --- a/security/integrity/evm/evm_crypto.c > >>>>>> +++ b/security/integrity/evm/evm_crypto.c > >>>>>> @@ -223,7 +223,7 @@ static int evm_calc_hmac_or_hash(struct dentry *dentry, > >>>>>> size_t req_xattr_value_len, > >>>>>> uint8_t type, struct evm_digest *data) > >>>>>> { > >>>>>> - struct inode *inode = d_backing_inode(dentry); > >>>>>> + struct inode *inode = d_real_inode(dentry); > >>>>>> struct xattr_list *xattr; > >>>>>> struct shash_desc *desc; > >>>>>> size_t xattr_size = 0; > >>>>> > >>>>> We need this patch when NOT activating CONFIG_OVERLAY_FS_METACOPY but > >>>>> when setting CONFIG_OVERLAY_FS_METACOPY=y it has to be reverted... I am > >>>>> not sure what the solution is. > >>>> > >>>> I think d_real_inode() does not work correctly for all its current users for > >>>> a metacopy file. > >>>> > >>>> I think the solution is to change d_real_inode() to return the data inode > >>>> and add another helper to get the metadata inode if needed. > >>>> I will post some patches for it. > >>> > >>> I thought that we may have to go through vfs_getattr() but even better > >>> if we don't because we don't have the file *file anywhere 'near'. > >>> > >>>> > >>>> However, I must say that I do not know if evm_calc_hmac_or_hash() > >>>> needs the lower data inode, the upper metadata inode or both. > >>> > >>> What it needs are data structures with mode bits, uid, and gid that stat > >>> in userspace would show. > >>> > >>> > >> > >> With or without metacopy enabled, an overlay inode st_uid st_gid st_mode > >> are always taken from the upper most inode which is what d_real_inode() > >> currently returns, so I do not understand what the problem is. > >> > > > > No, I was wrong. It is the other way around. > > d_real_inode() always returns the real data inode and you need the > > upper most real inode. > > > > You can try this: > > > > - struct inode *inode = d_backing_inode(dentry); > > + struct inode *inode = d_inode(d_real(dentry, false)); > > > > With the changes in: > > > > https://github.com/amir73il/linux/commits/overlayfs-devel/ > > > > Not thoroughly tested... > > The change + 3 topmost patches cherry-picked is unfortunately are > crashing for me. > I will look into it. But anyway, the patch I suggested above is not enough exactly because of the reason I told you earlier. Mimi's fix ("ima: detect changes to the backing overlay file") detects a change in d_real_inode(file_dentry(file)) in order to invalidate the IMA cache. Your change also invalidates EVM cache on a change in d_real_inode(file_dentry(file)) and that makes sense. But on "meta copy up" for example on chmod(), an upper inode with no data is created (a metacopy) and all the attributes and xattr are copied from lower inode. The data remains in the lower inode. At this point , the IMA cache and the EVM cache refer to two different inodes so you cannot share the same logic with IMA cache invalidation. My patches are meant to provide you with a helper e.g. d_real_meta_inode() that you could use to get the upper inode in the case of a metacopy, but IMA would still need to use the d_real_data_inode(). Is that explanation clear? Is it clear why I said that the problem is more complicated? Thanks, Amir.
On 2/1/24 07:10, Amir Goldstein wrote: > On Wed, Jan 31, 2024 at 7:46 PM Stefan Berger <stefanb@linux.ibm.com> wrote: >> >> >> >> On 1/31/24 12:23, Amir Goldstein wrote: >>> On Wed, Jan 31, 2024 at 5:54 PM Amir Goldstein <amir73il@gmail.com> wrote: >>>> >>>> On Wed, Jan 31, 2024 at 4:40 PM Stefan Berger <stefanb@linux.ibm.com> wrote: >>>>> >>>>> >>>>> >>>>> On 1/31/24 08:16, Amir Goldstein wrote: >>>>>> On Wed, Jan 31, 2024 at 4:11 AM Stefan Berger <stefanb@linux.ibm.com> wrote: >>>>>>> >>>>>>> >>>>>>> >>>>>>> On 1/30/24 16:46, Stefan Berger wrote: >>>>>>>> Changes to the file attribute (mode bits, uid, gid) on the lower layer >>>>>>>> are not take into account when d_backing_inode() is used when a file is >>>>>>>> accessed on the overlay layer and this file has not yet been copied up. >>>>>>>> This is because d_backing_inode() does not return the real inode of the >>>>>>>> lower layer but instead returns the backing inode which holds old file >>>>>>>> attributes. When the old file attributes are used for calculating the >>>>>>>> metadata hash then the expected hash is calculated and the file then >>>>>>>> mistakenly passes signature verification. Therefore, use d_real_inode() >>>>>>>> which returns the inode of the lower layer for as long as the file has >>>>>>>> not been copied up and returns the upper layer's inode otherwise. >>>>>>>> >>>>>>>> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com> >>>>>>>> --- >>>>>>>> security/integrity/evm/evm_crypto.c | 2 +- >>>>>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>>>>> >>>>>>>> diff --git a/security/integrity/evm/evm_crypto.c b/security/integrity/evm/evm_crypto.c >>>>>>>> index b1ffd4cc0b44..2e48fe54e899 100644 >>>>>>>> --- a/security/integrity/evm/evm_crypto.c >>>>>>>> +++ b/security/integrity/evm/evm_crypto.c >>>>>>>> @@ -223,7 +223,7 @@ static int evm_calc_hmac_or_hash(struct dentry *dentry, >>>>>>>> size_t req_xattr_value_len, >>>>>>>> uint8_t type, struct evm_digest *data) >>>>>>>> { >>>>>>>> - struct inode *inode = d_backing_inode(dentry); >>>>>>>> + struct inode *inode = d_real_inode(dentry); >>>>>>>> struct xattr_list *xattr; >>>>>>>> struct shash_desc *desc; >>>>>>>> size_t xattr_size = 0; >>>>>>> >>>>>>> We need this patch when NOT activating CONFIG_OVERLAY_FS_METACOPY but >>>>>>> when setting CONFIG_OVERLAY_FS_METACOPY=y it has to be reverted... I am >>>>>>> not sure what the solution is. >>>>>> >>>>>> I think d_real_inode() does not work correctly for all its current users for >>>>>> a metacopy file. >>>>>> >>>>>> I think the solution is to change d_real_inode() to return the data inode >>>>>> and add another helper to get the metadata inode if needed. >>>>>> I will post some patches for it. >>>>> >>>>> I thought that we may have to go through vfs_getattr() but even better >>>>> if we don't because we don't have the file *file anywhere 'near'. >>>>> >>>>>> >>>>>> However, I must say that I do not know if evm_calc_hmac_or_hash() >>>>>> needs the lower data inode, the upper metadata inode or both. >>>>> >>>>> What it needs are data structures with mode bits, uid, and gid that stat >>>>> in userspace would show. >>>>> >>>>> >>>> >>>> With or without metacopy enabled, an overlay inode st_uid st_gid st_mode >>>> are always taken from the upper most inode which is what d_real_inode() >>>> currently returns, so I do not understand what the problem is. >>>> >>> >>> No, I was wrong. It is the other way around. >>> d_real_inode() always returns the real data inode and you need the >>> upper most real inode. >>> >>> You can try this: >>> >>> - struct inode *inode = d_backing_inode(dentry); >>> + struct inode *inode = d_inode(d_real(dentry, false)); >>> >>> With the changes in: >>> >>> https://github.com/amir73il/linux/commits/overlayfs-devel/ >>> >>> Not thoroughly tested... >> >> The change + 3 topmost patches cherry-picked is unfortunately are >> crashing for me. >> > > I will look into it. > But anyway, the patch I suggested above is not enough exactly because > of the reason I told you earlier. > > Mimi's fix ("ima: detect changes to the backing overlay file") detects > a change in d_real_inode(file_dentry(file)) in order to invalidate the > IMA cache. > > Your change also invalidates EVM cache on a change in > d_real_inode(file_dentry(file)) and that makes sense. > > But on "meta copy up" for example on chmod(), an upper inode with no data > is created (a metacopy) and all the attributes and xattr are copied > from lower inode. > The data remains in the lower inode. > > At this point , the IMA cache and the EVM cache refer to two different inodes You mean they refer to different inodes because IMA cares about file content ("data remains in the lower inode:) and EVM cares about the metadata ("an upper inode with no data is created")? If so, I agree since the following line after copy-up with meatacopy enabled shows the proper GID is in the backing inode not the one return from d_real_inode(). If we knew that a meta copy has been done we could call d_backing_inode() in this case for access to mode bits, uid, and gid. + printk(KERN_INFO "real: GID: %d backing: GID: %d\n", + from_kgid(&init_user_ns, d_real_inode(dentry)->i_gid), + from_kgid(&init_user_ns, d_backing_inode(dentry)->i_gid)); + > so you cannot share the same logic with IMA cache invalidation. I thought we we would have to share the same logic since IMA and EVM would have to refer to the same inode also since IMA and EVM are strongly connected. So the file_inode(file), which is typically used for finding the iint, should be the same 'high level' inode for both EVM and IMA I thought. A different inode could then be used for file data and metadata. > > My patches are meant to provide you with a helper e.g. d_real_meta_inode() The patch providing this isn't there yet in overlayfs-devel, right? > that you could use to get the upper inode in the case of a metacopy, but > IMA would still need to use the d_real_data_inode(). That would be fine since we are only changing EVM code in this case. > > Is that explanation clear? Is it clear why I said that the problem is more > complicated? I think I understand it. Stefan > > Thanks, > Amir.
On Thu, Feb 1, 2024 at 3:37 PM Stefan Berger <stefanb@linux.ibm.com> wrote: > > > > On 2/1/24 07:10, Amir Goldstein wrote: > > On Wed, Jan 31, 2024 at 7:46 PM Stefan Berger <stefanb@linux.ibm.com> wrote: > >> > >> > >> > >> On 1/31/24 12:23, Amir Goldstein wrote: > >>> On Wed, Jan 31, 2024 at 5:54 PM Amir Goldstein <amir73il@gmail.com> wrote: > >>>> > >>>> On Wed, Jan 31, 2024 at 4:40 PM Stefan Berger <stefanb@linux.ibm.com> wrote: > >>>>> > >>>>> > >>>>> > >>>>> On 1/31/24 08:16, Amir Goldstein wrote: > >>>>>> On Wed, Jan 31, 2024 at 4:11 AM Stefan Berger <stefanb@linux.ibm.com> wrote: > >>>>>>> > >>>>>>> > >>>>>>> > >>>>>>> On 1/30/24 16:46, Stefan Berger wrote: > >>>>>>>> Changes to the file attribute (mode bits, uid, gid) on the lower layer > >>>>>>>> are not take into account when d_backing_inode() is used when a file is > >>>>>>>> accessed on the overlay layer and this file has not yet been copied up. > >>>>>>>> This is because d_backing_inode() does not return the real inode of the > >>>>>>>> lower layer but instead returns the backing inode which holds old file > >>>>>>>> attributes. When the old file attributes are used for calculating the > >>>>>>>> metadata hash then the expected hash is calculated and the file then > >>>>>>>> mistakenly passes signature verification. Therefore, use d_real_inode() > >>>>>>>> which returns the inode of the lower layer for as long as the file has > >>>>>>>> not been copied up and returns the upper layer's inode otherwise. > >>>>>>>> > >>>>>>>> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com> > >>>>>>>> --- > >>>>>>>> security/integrity/evm/evm_crypto.c | 2 +- > >>>>>>>> 1 file changed, 1 insertion(+), 1 deletion(-) > >>>>>>>> > >>>>>>>> diff --git a/security/integrity/evm/evm_crypto.c b/security/integrity/evm/evm_crypto.c > >>>>>>>> index b1ffd4cc0b44..2e48fe54e899 100644 > >>>>>>>> --- a/security/integrity/evm/evm_crypto.c > >>>>>>>> +++ b/security/integrity/evm/evm_crypto.c > >>>>>>>> @@ -223,7 +223,7 @@ static int evm_calc_hmac_or_hash(struct dentry *dentry, > >>>>>>>> size_t req_xattr_value_len, > >>>>>>>> uint8_t type, struct evm_digest *data) > >>>>>>>> { > >>>>>>>> - struct inode *inode = d_backing_inode(dentry); > >>>>>>>> + struct inode *inode = d_real_inode(dentry); > >>>>>>>> struct xattr_list *xattr; > >>>>>>>> struct shash_desc *desc; > >>>>>>>> size_t xattr_size = 0; > >>>>>>> > >>>>>>> We need this patch when NOT activating CONFIG_OVERLAY_FS_METACOPY but > >>>>>>> when setting CONFIG_OVERLAY_FS_METACOPY=y it has to be reverted... I am > >>>>>>> not sure what the solution is. > >>>>>> > >>>>>> I think d_real_inode() does not work correctly for all its current users for > >>>>>> a metacopy file. > >>>>>> > >>>>>> I think the solution is to change d_real_inode() to return the data inode > >>>>>> and add another helper to get the metadata inode if needed. > >>>>>> I will post some patches for it. > >>>>> > >>>>> I thought that we may have to go through vfs_getattr() but even better > >>>>> if we don't because we don't have the file *file anywhere 'near'. > >>>>> > >>>>>> > >>>>>> However, I must say that I do not know if evm_calc_hmac_or_hash() > >>>>>> needs the lower data inode, the upper metadata inode or both. > >>>>> > >>>>> What it needs are data structures with mode bits, uid, and gid that stat > >>>>> in userspace would show. > >>>>> > >>>>> > >>>> > >>>> With or without metacopy enabled, an overlay inode st_uid st_gid st_mode > >>>> are always taken from the upper most inode which is what d_real_inode() > >>>> currently returns, so I do not understand what the problem is. > >>>> > >>> > >>> No, I was wrong. It is the other way around. > >>> d_real_inode() always returns the real data inode and you need the > >>> upper most real inode. > >>> > >>> You can try this: > >>> > >>> - struct inode *inode = d_backing_inode(dentry); > >>> + struct inode *inode = d_inode(d_real(dentry, false)); > >>> > >>> With the changes in: > >>> > >>> https://github.com/amir73il/linux/commits/overlayfs-devel/ > >>> > >>> Not thoroughly tested... > >> > >> The change + 3 topmost patches cherry-picked is unfortunately are > >> crashing for me. > >> > > > > I will look into it. > > But anyway, the patch I suggested above is not enough exactly because > > of the reason I told you earlier. > > > > Mimi's fix ("ima: detect changes to the backing overlay file") detects > > a change in d_real_inode(file_dentry(file)) in order to invalidate the > > IMA cache. > > > > Your change also invalidates EVM cache on a change in > > d_real_inode(file_dentry(file)) and that makes sense. > > > > But on "meta copy up" for example on chmod(), an upper inode with no data > > is created (a metacopy) and all the attributes and xattr are copied > > from lower inode. > > The data remains in the lower inode. > > > > At this point , the IMA cache and the EVM cache refer to two different inodes > > You mean they refer to different inodes because IMA cares about file > content ("data remains in the lower inode:) and EVM cares about the > metadata ("an upper inode with no data is created")? If so, I agree Correct. > since the following line after copy-up with meatacopy enabled shows the > proper GID is in the backing inode not the one return from > d_real_inode(). If we knew that a meta copy has been done we could call > d_backing_inode() in this case for access to mode bits, uid, and gid. > You should be able to use d_real_meta_inode(dentry) != d_real_inode(dentry) to figure that out. > + printk(KERN_INFO "real: GID: %d backing: GID: %d\n", > + from_kgid(&init_user_ns, d_real_inode(dentry)->i_gid), > + from_kgid(&init_user_ns, d_backing_inode(dentry)->i_gid)); > + > > > so you cannot share the same logic with IMA cache invalidation. > > I thought we we would have to share the same logic since IMA and EVM > would have to refer to the same inode also since IMA and EVM are > strongly connected. So the file_inode(file), which is typically used for > finding the iint, should be the same 'high level' inode for both EVM and > IMA I thought. A different inode could then be used for file data and > metadata. > > > > > My patches are meant to provide you with a helper e.g. d_real_meta_inode() > > The patch providing this isn't there yet in overlayfs-devel, right? It's there just not spelled out with these helper names: d_real_meta_inode(d) := d_inode(d_real(dentry, false)) d_real_data_inode(d) := d_inode(d_real(dentry, true)) d_real_inode(d) := d_real_data_inode(d) I think this use case is pretty specific to EVM, so I don't think I will actually define these d_real_*_inode() helpers. > > > that you could use to get the upper inode in the case of a metacopy, but > > IMA would still need to use the d_real_data_inode(). > > That would be fine since we are only changing EVM code in this case. Yes, using those overlayfs APIs requires understanding of what they mean and knowing how to test the affected use cases. This is not something very common for other subsystem developers. > > > > Is that explanation clear? Is it clear why I said that the problem is more > > complicated? > > I think I understand it. > I think I am also starting to understand the expectation from IMA/EVM over overlayfs ;) Anyway, let me see if I can fix the problem in my WIP branch. Thanks, Amir.
On 2/1/24 09:11, Amir Goldstein wrote: > On Thu, Feb 1, 2024 at 3:37 PM Stefan Berger <stefanb@linux.ibm.com> wrote: >> >> >> >> On 2/1/24 07:10, Amir Goldstein wrote: >>> On Wed, Jan 31, 2024 at 7:46 PM Stefan Berger <stefanb@linux.ibm.com> wrote: >>>> >>>> >>>> >>>> On 1/31/24 12:23, Amir Goldstein wrote: >>>>> On Wed, Jan 31, 2024 at 5:54 PM Amir Goldstein <amir73il@gmail.com> wrote: >>>>>> >>>>>> On Wed, Jan 31, 2024 at 4:40 PM Stefan Berger <stefanb@linux.ibm.com> wrote: >>>>>>> >>>>>>> >>>>>>> >>>>>>> On 1/31/24 08:16, Amir Goldstein wrote: >>>>>>>> On Wed, Jan 31, 2024 at 4:11 AM Stefan Berger <stefanb@linux.ibm.com> wrote: >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> On 1/30/24 16:46, Stefan Berger wrote: >>>>>>>>>> Changes to the file attribute (mode bits, uid, gid) on the lower layer >>>>>>>>>> are not take into account when d_backing_inode() is used when a file is >>>>>>>>>> accessed on the overlay layer and this file has not yet been copied up. >>>>>>>>>> This is because d_backing_inode() does not return the real inode of the >>>>>>>>>> lower layer but instead returns the backing inode which holds old file >>>>>>>>>> attributes. When the old file attributes are used for calculating the >>>>>>>>>> metadata hash then the expected hash is calculated and the file then >>>>>>>>>> mistakenly passes signature verification. Therefore, use d_real_inode() >>>>>>>>>> which returns the inode of the lower layer for as long as the file has >>>>>>>>>> not been copied up and returns the upper layer's inode otherwise. >>>>>>>>>> >>>>>>>>>> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com> >>>>>>>>>> --- >>>>>>>>>> security/integrity/evm/evm_crypto.c | 2 +- >>>>>>>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>>>>>>> >>>>>>>>>> diff --git a/security/integrity/evm/evm_crypto.c b/security/integrity/evm/evm_crypto.c >>>>>>>>>> index b1ffd4cc0b44..2e48fe54e899 100644 >>>>>>>>>> --- a/security/integrity/evm/evm_crypto.c >>>>>>>>>> +++ b/security/integrity/evm/evm_crypto.c >>>>>>>>>> @@ -223,7 +223,7 @@ static int evm_calc_hmac_or_hash(struct dentry *dentry, >>>>>>>>>> size_t req_xattr_value_len, >>>>>>>>>> uint8_t type, struct evm_digest *data) >>>>>>>>>> { >>>>>>>>>> - struct inode *inode = d_backing_inode(dentry); >>>>>>>>>> + struct inode *inode = d_real_inode(dentry); >>>>>>>>>> struct xattr_list *xattr; >>>>>>>>>> struct shash_desc *desc; >>>>>>>>>> size_t xattr_size = 0; >>>>>>>>> >>>>>>>>> We need this patch when NOT activating CONFIG_OVERLAY_FS_METACOPY but >>>>>>>>> when setting CONFIG_OVERLAY_FS_METACOPY=y it has to be reverted... I am >>>>>>>>> not sure what the solution is. >>>>>>>> >>>>>>>> I think d_real_inode() does not work correctly for all its current users for >>>>>>>> a metacopy file. >>>>>>>> >>>>>>>> I think the solution is to change d_real_inode() to return the data inode >>>>>>>> and add another helper to get the metadata inode if needed. >>>>>>>> I will post some patches for it. >>>>>>> >>>>>>> I thought that we may have to go through vfs_getattr() but even better >>>>>>> if we don't because we don't have the file *file anywhere 'near'. >>>>>>> >>>>>>>> >>>>>>>> However, I must say that I do not know if evm_calc_hmac_or_hash() >>>>>>>> needs the lower data inode, the upper metadata inode or both. >>>>>>> >>>>>>> What it needs are data structures with mode bits, uid, and gid that stat >>>>>>> in userspace would show. >>>>>>> >>>>>>> >>>>>> >>>>>> With or without metacopy enabled, an overlay inode st_uid st_gid st_mode >>>>>> are always taken from the upper most inode which is what d_real_inode() >>>>>> currently returns, so I do not understand what the problem is. >>>>>> >>>>> >>>>> No, I was wrong. It is the other way around. >>>>> d_real_inode() always returns the real data inode and you need the >>>>> upper most real inode. >>>>> >>>>> You can try this: >>>>> >>>>> - struct inode *inode = d_backing_inode(dentry); >>>>> + struct inode *inode = d_inode(d_real(dentry, false)); >>>>> >>>>> With the changes in: >>>>> >>>>> https://github.com/amir73il/linux/commits/overlayfs-devel/ >>>>> >>>>> Not thoroughly tested... >>>> >>>> The change + 3 topmost patches cherry-picked is unfortunately are >>>> crashing for me. >>>> >>> >>> I will look into it. >>> But anyway, the patch I suggested above is not enough exactly because >>> of the reason I told you earlier. >>> >>> Mimi's fix ("ima: detect changes to the backing overlay file") detects >>> a change in d_real_inode(file_dentry(file)) in order to invalidate the >>> IMA cache. >>> >>> Your change also invalidates EVM cache on a change in >>> d_real_inode(file_dentry(file)) and that makes sense. >>> >>> But on "meta copy up" for example on chmod(), an upper inode with no data >>> is created (a metacopy) and all the attributes and xattr are copied >>> from lower inode. >>> The data remains in the lower inode. >>> >>> At this point , the IMA cache and the EVM cache refer to two different inodes >> >> You mean they refer to different inodes because IMA cares about file >> content ("data remains in the lower inode:) and EVM cares about the >> metadata ("an upper inode with no data is created")? If so, I agree > > Correct. > >> since the following line after copy-up with meatacopy enabled shows the >> proper GID is in the backing inode not the one return from >> d_real_inode(). If we knew that a meta copy has been done we could call >> d_backing_inode() in this case for access to mode bits, uid, and gid. >> > > You should be able to use > d_real_meta_inode(dentry) != d_real_inode(dentry) to figure that out. > >> + printk(KERN_INFO "real: GID: %d backing: GID: %d\n", >> + from_kgid(&init_user_ns, d_real_inode(dentry)->i_gid), >> + from_kgid(&init_user_ns, d_backing_inode(dentry)->i_gid)); >> + >> >> > so you cannot share the same logic with IMA cache invalidation. >> >> I thought we we would have to share the same logic since IMA and EVM >> would have to refer to the same inode also since IMA and EVM are >> strongly connected. So the file_inode(file), which is typically used for >> finding the iint, should be the same 'high level' inode for both EVM and >> IMA I thought. A different inode could then be used for file data and >> metadata. >> >>> >>> My patches are meant to provide you with a helper e.g. d_real_meta_inode() >> >> The patch providing this isn't there yet in overlayfs-devel, right? > > It's there just not spelled out with these helper names: > > d_real_meta_inode(d) := d_inode(d_real(dentry, false)) > d_real_data_inode(d) := d_inode(d_real(dentry, true)) > d_real_inode(d) := d_real_data_inode(d) > > I think this use case is pretty specific to EVM, so I don't think > I will actually define these d_real_*_inode() helpers. > >> >>> that you could use to get the upper inode in the case of a metacopy, but >>> IMA would still need to use the d_real_data_inode(). >> >> That would be fine since we are only changing EVM code in this case. > > Yes, using those overlayfs APIs requires understanding of what they mean > and knowing how to test the affected use cases. > This is not something very common for other subsystem developers. > >>> >>> Is that explanation clear? Is it clear why I said that the problem is more >>> complicated? >> >> I think I understand it. >> > > I think I am also starting to understand the expectation from IMA/EVM > over overlayfs ;) > > Anyway, let me see if I can fix the problem in my WIP branch. The good news is that with your two patches applied : 3b0ed3977dd2 (HEAD) fs: make file_dentry() a simple accessor b5ccc40f3d50 fs: remove the inode argument to ->d_real() method and your suggested change to this patch : - struct inode *inode = d_real_inode(dentry); + struct inode *inode = d_inode(d_real(dentry, false));; The test cases are now passing with and without metacopy enabled. Yay! Your 3rd patch causes crashes.. Stefan > > Thanks, > Amir.
On Thu, Feb 1, 2024 at 10:35 PM Stefan Berger <stefanb@linux.ibm.com> wrote: > > > > On 2/1/24 09:11, Amir Goldstein wrote: > > On Thu, Feb 1, 2024 at 3:37 PM Stefan Berger <stefanb@linux.ibm.com> wrote: > >> > >> > >> > >> On 2/1/24 07:10, Amir Goldstein wrote: > >>> On Wed, Jan 31, 2024 at 7:46 PM Stefan Berger <stefanb@linux.ibm.com> wrote: > >>>> > >>>> > >>>> > >>>> On 1/31/24 12:23, Amir Goldstein wrote: > >>>>> On Wed, Jan 31, 2024 at 5:54 PM Amir Goldstein <amir73il@gmail.com> wrote: > >>>>>> > >>>>>> On Wed, Jan 31, 2024 at 4:40 PM Stefan Berger <stefanb@linux.ibm.com> wrote: > >>>>>>> > >>>>>>> > >>>>>>> > >>>>>>> On 1/31/24 08:16, Amir Goldstein wrote: > >>>>>>>> On Wed, Jan 31, 2024 at 4:11 AM Stefan Berger <stefanb@linux.ibm.com> wrote: > >>>>>>>>> > >>>>>>>>> > >>>>>>>>> > >>>>>>>>> On 1/30/24 16:46, Stefan Berger wrote: > >>>>>>>>>> Changes to the file attribute (mode bits, uid, gid) on the lower layer > >>>>>>>>>> are not take into account when d_backing_inode() is used when a file is > >>>>>>>>>> accessed on the overlay layer and this file has not yet been copied up. > >>>>>>>>>> This is because d_backing_inode() does not return the real inode of the > >>>>>>>>>> lower layer but instead returns the backing inode which holds old file > >>>>>>>>>> attributes. When the old file attributes are used for calculating the > >>>>>>>>>> metadata hash then the expected hash is calculated and the file then > >>>>>>>>>> mistakenly passes signature verification. Therefore, use d_real_inode() > >>>>>>>>>> which returns the inode of the lower layer for as long as the file has > >>>>>>>>>> not been copied up and returns the upper layer's inode otherwise. > >>>>>>>>>> > >>>>>>>>>> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com> > >>>>>>>>>> --- > >>>>>>>>>> security/integrity/evm/evm_crypto.c | 2 +- > >>>>>>>>>> 1 file changed, 1 insertion(+), 1 deletion(-) > >>>>>>>>>> > >>>>>>>>>> diff --git a/security/integrity/evm/evm_crypto.c b/security/integrity/evm/evm_crypto.c > >>>>>>>>>> index b1ffd4cc0b44..2e48fe54e899 100644 > >>>>>>>>>> --- a/security/integrity/evm/evm_crypto.c > >>>>>>>>>> +++ b/security/integrity/evm/evm_crypto.c > >>>>>>>>>> @@ -223,7 +223,7 @@ static int evm_calc_hmac_or_hash(struct dentry *dentry, > >>>>>>>>>> size_t req_xattr_value_len, > >>>>>>>>>> uint8_t type, struct evm_digest *data) > >>>>>>>>>> { > >>>>>>>>>> - struct inode *inode = d_backing_inode(dentry); > >>>>>>>>>> + struct inode *inode = d_real_inode(dentry); > >>>>>>>>>> struct xattr_list *xattr; > >>>>>>>>>> struct shash_desc *desc; > >>>>>>>>>> size_t xattr_size = 0; > >>>>>>>>> > >>>>>>>>> We need this patch when NOT activating CONFIG_OVERLAY_FS_METACOPY but > >>>>>>>>> when setting CONFIG_OVERLAY_FS_METACOPY=y it has to be reverted... I am > >>>>>>>>> not sure what the solution is. > >>>>>>>> > >>>>>>>> I think d_real_inode() does not work correctly for all its current users for > >>>>>>>> a metacopy file. > >>>>>>>> > >>>>>>>> I think the solution is to change d_real_inode() to return the data inode > >>>>>>>> and add another helper to get the metadata inode if needed. > >>>>>>>> I will post some patches for it. > >>>>>>> > >>>>>>> I thought that we may have to go through vfs_getattr() but even better > >>>>>>> if we don't because we don't have the file *file anywhere 'near'. > >>>>>>> > >>>>>>>> > >>>>>>>> However, I must say that I do not know if evm_calc_hmac_or_hash() > >>>>>>>> needs the lower data inode, the upper metadata inode or both. > >>>>>>> > >>>>>>> What it needs are data structures with mode bits, uid, and gid that stat > >>>>>>> in userspace would show. > >>>>>>> > >>>>>>> > >>>>>> > >>>>>> With or without metacopy enabled, an overlay inode st_uid st_gid st_mode > >>>>>> are always taken from the upper most inode which is what d_real_inode() > >>>>>> currently returns, so I do not understand what the problem is. > >>>>>> > >>>>> > >>>>> No, I was wrong. It is the other way around. > >>>>> d_real_inode() always returns the real data inode and you need the > >>>>> upper most real inode. > >>>>> > >>>>> You can try this: > >>>>> > >>>>> - struct inode *inode = d_backing_inode(dentry); > >>>>> + struct inode *inode = d_inode(d_real(dentry, false)); > >>>>> > >>>>> With the changes in: > >>>>> > >>>>> https://github.com/amir73il/linux/commits/overlayfs-devel/ > >>>>> > >>>>> Not thoroughly tested... > >>>> > >>>> The change + 3 topmost patches cherry-picked is unfortunately are > >>>> crashing for me. > >>>> > >>> > >>> I will look into it. > >>> But anyway, the patch I suggested above is not enough exactly because > >>> of the reason I told you earlier. > >>> > >>> Mimi's fix ("ima: detect changes to the backing overlay file") detects > >>> a change in d_real_inode(file_dentry(file)) in order to invalidate the > >>> IMA cache. > >>> > >>> Your change also invalidates EVM cache on a change in > >>> d_real_inode(file_dentry(file)) and that makes sense. > >>> > >>> But on "meta copy up" for example on chmod(), an upper inode with no data > >>> is created (a metacopy) and all the attributes and xattr are copied > >>> from lower inode. > >>> The data remains in the lower inode. > >>> > >>> At this point , the IMA cache and the EVM cache refer to two different inodes > >> > >> You mean they refer to different inodes because IMA cares about file > >> content ("data remains in the lower inode:) and EVM cares about the > >> metadata ("an upper inode with no data is created")? If so, I agree > > > > Correct. > > > >> since the following line after copy-up with meatacopy enabled shows the > >> proper GID is in the backing inode not the one return from > >> d_real_inode(). If we knew that a meta copy has been done we could call > >> d_backing_inode() in this case for access to mode bits, uid, and gid. > >> > > > > You should be able to use > > d_real_meta_inode(dentry) != d_real_inode(dentry) to figure that out. > > > >> + printk(KERN_INFO "real: GID: %d backing: GID: %d\n", > >> + from_kgid(&init_user_ns, d_real_inode(dentry)->i_gid), > >> + from_kgid(&init_user_ns, d_backing_inode(dentry)->i_gid)); > >> + > >> > >> > so you cannot share the same logic with IMA cache invalidation. > >> > >> I thought we we would have to share the same logic since IMA and EVM > >> would have to refer to the same inode also since IMA and EVM are > >> strongly connected. So the file_inode(file), which is typically used for > >> finding the iint, should be the same 'high level' inode for both EVM and > >> IMA I thought. A different inode could then be used for file data and > >> metadata. > >> > >>> > >>> My patches are meant to provide you with a helper e.g. d_real_meta_inode() > >> > >> The patch providing this isn't there yet in overlayfs-devel, right? > > > > It's there just not spelled out with these helper names: > > > > d_real_meta_inode(d) := d_inode(d_real(dentry, false)) > > d_real_data_inode(d) := d_inode(d_real(dentry, true)) > > d_real_inode(d) := d_real_data_inode(d) > > > > I think this use case is pretty specific to EVM, so I don't think > > I will actually define these d_real_*_inode() helpers. > > > >> > >>> that you could use to get the upper inode in the case of a metacopy, but > >>> IMA would still need to use the d_real_data_inode(). > >> > >> That would be fine since we are only changing EVM code in this case. > > > > Yes, using those overlayfs APIs requires understanding of what they mean > > and knowing how to test the affected use cases. > > This is not something very common for other subsystem developers. > > > >>> > >>> Is that explanation clear? Is it clear why I said that the problem is more > >>> complicated? > >> > >> I think I understand it. > >> > > > > I think I am also starting to understand the expectation from IMA/EVM > > over overlayfs ;) > > > > Anyway, let me see if I can fix the problem in my WIP branch. > > The good news is that with your two patches applied : > > 3b0ed3977dd2 (HEAD) fs: make file_dentry() a simple accessor > b5ccc40f3d50 fs: remove the inode argument to ->d_real() method Doh! good catch! This first fix commit was buggy. Pushed a new fixed version: * 4d76c382bf12 - (github/overlayfs-devel) fs: remove the inode argument to ->d_real() method * 2cadd1b25485 - fs: make file_dentry() a simple accessor * 1c5e7db8e1b2 - (github/ovl-fixes) remap_range: merge do_clone_file_range() into vfs_clone_file_range() > > and your suggested change to this patch : > > - struct inode *inode = d_real_inode(dentry); > + struct inode *inode = d_inode(d_real(dentry, false));; > In the new version I change the API to use an enum instead of bool, e.g.: struct inode *inode = d_inode(d_real(dentry, D_REAL_METADATA)); This catches in build time and in run time, callers that were not converted to the new API. > The test cases are now passing with and without metacopy enabled. Yay! Too soon to be happy. I guess you are missing a test for the following case: 1. file was meta copied up (change is detected) 2. the lower file that contains the data is being changed (change is not detected) At #2 the change is not detected, because the inode version of the lower inode is not checked. I think the only way for you to cover this case is to store versions of both real data and real metadata inodes... Let me know if this version work for you and I will added you Tested-by when I post it. Thanks, Amir.
On 2/2/24 04:24, Amir Goldstein wrote: > On Thu, Feb 1, 2024 at 10:35 PM Stefan Berger <stefanb@linux.ibm.com> wrote: > >> >> and your suggested change to this patch : >> >> - struct inode *inode = d_real_inode(dentry); >> + struct inode *inode = d_inode(d_real(dentry, false));; >> > > In the new version I change the API to use an enum instead of bool, e.g.: > > struct inode *inode = d_inode(d_real(dentry, D_REAL_METADATA)); Thanks. I will use it. > > This catches in build time and in run time, callers that were not converted > to the new API. > >> The test cases are now passing with and without metacopy enabled. Yay! > > Too soon to be happy. > I guess you are missing a test for the following case: > 1. file was meta copied up (change is detected) > 2. the lower file that contains the data is being changed (change is > not detected) Right. Though it seems there's something wrong with overlayfs as well after appending a byte to the file on the lower. -rwxr-xr-x 1 0 0 25 Feb 2 14:55 /ext4.mount/lower/test_rsa_portable2 -rwxr-xr-x 1 0 0 24 Feb 2 14:55 /ext4.mount/overlay/test_rsa_portable2 bb16aa5350bcc8863da1a873c846fec9281842d9 /ext4.mount/lower/test_rsa_portable2 bb16aa5350bcc8863da1a873c846fec9281842d9 /ext4.mount/overlay/test_rsa_portable2 We have a hash collision on a file with 24 bytes and the underlying one with 25 byte. (-; :-) Stefan
On Fri, Feb 2, 2024 at 4:59 PM Stefan Berger <stefanb@linux.ibm.com> wrote: > > > > On 2/2/24 04:24, Amir Goldstein wrote: > > On Thu, Feb 1, 2024 at 10:35 PM Stefan Berger <stefanb@linux.ibm.com> wrote: > > > > >> > >> and your suggested change to this patch : > >> > >> - struct inode *inode = d_real_inode(dentry); > >> + struct inode *inode = d_inode(d_real(dentry, false));; > >> > > > > In the new version I change the API to use an enum instead of bool, e.g.: > > > > struct inode *inode = d_inode(d_real(dentry, D_REAL_METADATA)); > > Thanks. I will use it. > > > > > This catches in build time and in run time, callers that were not converted > > to the new API. > > > >> The test cases are now passing with and without metacopy enabled. Yay! > > > > Too soon to be happy. > > I guess you are missing a test for the following case: > > 1. file was meta copied up (change is detected) > > 2. the lower file that contains the data is being changed (change is > > not detected) > > Right. Though it seems there's something wrong with overlayfs as well > after appending a byte to the file on the lower. > > -rwxr-xr-x 1 0 0 25 Feb 2 14:55 > /ext4.mount/lower/test_rsa_portable2 > -rwxr-xr-x 1 0 0 24 Feb 2 14:55 > /ext4.mount/overlay/test_rsa_portable2 > bb16aa5350bcc8863da1a873c846fec9281842d9 > /ext4.mount/lower/test_rsa_portable2 > bb16aa5350bcc8863da1a873c846fec9281842d9 > /ext4.mount/overlay/test_rsa_portable2 > > We have a hash collision on a file with 24 bytes and the underlying one > with 25 byte. (-; :-) https://docs.kernel.org/filesystems/overlayfs.html#changes-to-underlying-filesystems If you modify the lower file underneath overlayfs, you get no guarantee from overlayfs about expected results. This makes your work more challenging. Thanks, Amir.
On 2/2/24 10:51, Amir Goldstein wrote: > On Fri, Feb 2, 2024 at 4:59 PM Stefan Berger <stefanb@linux.ibm.com> wrote: >> >> >> >> On 2/2/24 04:24, Amir Goldstein wrote: >>> On Thu, Feb 1, 2024 at 10:35 PM Stefan Berger <stefanb@linux.ibm.com> wrote: >> >>> >>>> >>>> and your suggested change to this patch : >>>> >>>> - struct inode *inode = d_real_inode(dentry); >>>> + struct inode *inode = d_inode(d_real(dentry, false));; >>>> >>> >>> In the new version I change the API to use an enum instead of bool, e.g.: >>> >>> struct inode *inode = d_inode(d_real(dentry, D_REAL_METADATA)); >> >> Thanks. I will use it. >> >>> >>> This catches in build time and in run time, callers that were not converted >>> to the new API. >>> >>>> The test cases are now passing with and without metacopy enabled. Yay! >>> >>> Too soon to be happy. >>> I guess you are missing a test for the following case: >>> 1. file was meta copied up (change is detected) >>> 2. the lower file that contains the data is being changed (change is >>> not detected) >> >> Right. Though it seems there's something wrong with overlayfs as well >> after appending a byte to the file on the lower. >> >> -rwxr-xr-x 1 0 0 25 Feb 2 14:55 >> /ext4.mount/lower/test_rsa_portable2 >> -rwxr-xr-x 1 0 0 24 Feb 2 14:55 >> /ext4.mount/overlay/test_rsa_portable2 >> bb16aa5350bcc8863da1a873c846fec9281842d9 >> /ext4.mount/lower/test_rsa_portable2 >> bb16aa5350bcc8863da1a873c846fec9281842d9 >> /ext4.mount/overlay/test_rsa_portable2 >> >> We have a hash collision on a file with 24 bytes and the underlying one >> with 25 byte. (-; :-) > > https://docs.kernel.org/filesystems/overlayfs.html#changes-to-underlying-filesystems > > If you modify the lower file underneath overlayfs, you get no > guarantee from overlayfs about expected results. > > This makes your work more challenging. The odd thing is my updated test case '2' seems to indicate that everything already works as expected with CONFIG_OVERLAY_FS_METACOPY=y. After causing copy-up of metadata changes to the file content on the lower layer still cause permission error to file execution on the overlay layer and after restoring the file content on the lower the file on the overlay again runs as expected. The file content change + copy-up of file content also has completely decoupled the lower file from the file on the overlay and changes to the file on the lower cause no more file execution rejections on the overlay. Stefan > > Thanks, > Amir.
> The odd thing is my updated test case '2' seems to indicate that > everything already works as expected with CONFIG_OVERLAY_FS_METACOPY=y. > After causing copy-up of metadata changes to the file content on the > lower layer still cause permission error to file execution on the > overlay layer and after restoring the file content on the lower the file > on the overlay again runs as expected. The file content change + copy-up > of file content also has completely decoupled the lower file from the > file on the overlay and changes to the file on the lower cause no more > file execution rejections on the overlay. > Sorry, you lost me. The combination of IMA+EVM+OVL must be too complicated to explain in plain language without an explicit test spelled out... When you write "The file content change + copy-up of file content also has completely decoupled the lower file from the file on the overlay", what do you mean by "copy up of the file content"? Why was the file content copied up? I was asking about use case that only metadata was copied up but lower file content, which is still the content of the ovl file was changed underneath ovl - this case does not cause data content to be copied up. I don't think we understand each other. Thanks, Amir.
On 2/2/24 11:17, Amir Goldstein wrote: >> The odd thing is my updated test case '2' seems to indicate that >> everything already works as expected with CONFIG_OVERLAY_FS_METACOPY=y. >> After causing copy-up of metadata changes to the file content on the >> lower layer still cause permission error to file execution on the >> overlay layer and after restoring the file content on the lower the file >> on the overlay again runs as expected. The file content change + copy-up >> of file content also has completely decoupled the lower file from the >> file on the overlay and changes to the file on the lower cause no more >> file execution rejections on the overlay. >> > > Sorry, you lost me. > The combination of IMA+EVM+OVL must be too complicated to > explain in plain language without an explicit test spelled out... > > When you write "The file content change + copy-up of file content also > has completely decoupled the lower file from the file on the overlay", > what do you mean by "copy up of the file content"? > Why was the file content copied up? The file was copied up by appending a byte to the file on the 'overlay'. > I was asking about use case that only metadata was copied up but > lower file content, which is still the content of the ovl file was changed > underneath ovl - this case does not cause data content to be copied up. > > I don't think we understand each other. One of the test cases I also have is appending a byte to the file on the 'lower'. At this point in the test one can detect whether CONFIG_OVERLAY_FS_METACOPY is enabled by checking the sha1 of the files on the lower and overlay layers and comparing their hashes. If they are equal then CONFIG_OVERLAY_FS_METACOPY is enabled since previously in the test file metadata on the overlay layer was already changed, which in the CONFIG_OVERLAY_FS_METACOPY=y case only caused a copy-up of metadata. So, when trying to execute the file on the overlay layer the file cannot be executed due to the file content change on the lower layer (IMA should be the one detecting this, need to check) still 'shining through'. After restoring the file content on the lower layer the file again executes on the 'overlay' layer - as expected. Stefan > > Thanks, > Amir.
diff --git a/security/integrity/evm/evm_crypto.c b/security/integrity/evm/evm_crypto.c index b1ffd4cc0b44..2e48fe54e899 100644 --- a/security/integrity/evm/evm_crypto.c +++ b/security/integrity/evm/evm_crypto.c @@ -223,7 +223,7 @@ static int evm_calc_hmac_or_hash(struct dentry *dentry, size_t req_xattr_value_len, uint8_t type, struct evm_digest *data) { - struct inode *inode = d_backing_inode(dentry); + struct inode *inode = d_real_inode(dentry); struct xattr_list *xattr; struct shash_desc *desc; size_t xattr_size = 0;
Changes to the file attribute (mode bits, uid, gid) on the lower layer are not take into account when d_backing_inode() is used when a file is accessed on the overlay layer and this file has not yet been copied up. This is because d_backing_inode() does not return the real inode of the lower layer but instead returns the backing inode which holds old file attributes. When the old file attributes are used for calculating the metadata hash then the expected hash is calculated and the file then mistakenly passes signature verification. Therefore, use d_real_inode() which returns the inode of the lower layer for as long as the file has not been copied up and returns the upper layer's inode otherwise. Signed-off-by: Stefan Berger <stefanb@linux.ibm.com> --- security/integrity/evm/evm_crypto.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)