On Wed, 2023-09-13 at 15:09 +0300, Amir Goldstein wrote: > On Wed, Sep 13, 2023 at 10:38 AM Amir Goldstein <amir73il@xxxxxxxxx> wrote: > > > > When storing IMA xattr on an overlayfs inode, the xattr is actually > > stored in the inode of the underlying (a.k.a real) filesystem, so there > > is an ambiguity whether this IMA xattr describes the integrity of the > > overlayfs inode or the real inode. > > > > For this reason and other reasons, IMA is not supported on overlayfs, > > in the sense that integrity checking on the overlayfs inode/file/path > > do not work correctly and have undefined behavior and the IMA xattr > > always describes the integrity of the real inode. > > > > When a user operates on an overlayfs file, whose underlying real file > > has IMA enabled, IMA should always operate on the real path and not > > on the overlayfs path. > > > > IMA code already uses the helper file_dentry() to get the dentry > > of the real file. Dereferencing file->f_path directly means that IMA > > will operate on the overlayfs inode, which is wrong. > > > > Therefore, all dereferences to f_path were converted to use the > > file_real_path() helper. Thanks, Amir. This sounds right. > > > > Reported-by: syzbot+a67fc5321ffb4b311c98@xxxxxxxxxxxxxxxxxxxxxxxxx > > Closes: https://lore.kernel.org/linux-unionfs/0000000000005bd097060530b758@xxxxxxxxxx/ > > Fixes: db1d1e8b9867 ("IMA: use vfs_getattr_nosec to get the i_version") > > Cc: Christian Brauner <brauner@xxxxxxxxxx> > > Cc: Jeff Layton <jlayton@xxxxxxxxxx> > > Cc: Goldwyn Rodrigues <rgoldwyn@xxxxxxxx> > > Signed-off-by: Amir Goldstein <amir73il@xxxxxxxxx> > > --- > > > > Mimi, > > > > Some of the wrong f_path dereferences are much older than the Fixes > > commit, but they did not have as big an impact as the wrong f_path > > dereference that the Fixes commit introduced. > > > > For example, commit a408e4a86b36 ("ima: open a new file instance if no > > read permissions") worked because reading the content of the overlayfs > > file has the same result as reading the content of the real file, but it > > is actually the real file integrity that we want to verify. > > > > Anyway, the real path information, that is now available via the > > file_real_path() helper, was not available in IMA integrity check context > > at the time that commit a408e4a86b36 was merged. > > Only problem is that fix did not resolve the syzbot bug, which > seems to do the IMA integrity check on overlayfs file (not sure). > > I am pretty sure that this patch fixes "a bug" when IMA is on the filesystem > under overlayfs and this is a pretty important use case. Agreed. > But I guess there are still issues with IMA over overlayfs and this is not > the only one. Sigh > Is this really a use case that needs to be supported? > Isn't the newly added SB_I_IMA_UNVERIFIABLE_SIGNATUREh flag > a hint that IMA on overlayfs is not a good idea at all? With SB_I_IMA_UNVERIFIABLE_SIGNATURE enabled for overlayfs, signature verification will then fail immediately for all overlayfs files in policy. I don't think that's the right solution. Verification should be limited to when the overlayfs file is the same as the underlying backing store, the real inode, not the overlay upper files. -- Thanks, Mimi
