Received-SPF: pass (google.com: domain of linux-kernel+bounces-143240-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <89b4fb29-5906-4b21-8b5b-6b340701ffe4@linux.ibm.com>
Date: Fri, 12 Apr 2024 15:08:54 -0400
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [RFC 2/2] ima: Fix detection of read/write violations on stacked
 filesystems
Content-Language: en-US
To: Amir Goldstein <amir73il@gmail.com>
Cc: linux-integrity@vger.kernel.org, linux-unionfs@vger.kernel.org,
        linux-kernel@vger.kernel.org, zohar@linux.ibm.com,
        roberto.sassu@huawei.com, miklos@szeredi.hu,
        Christian Brauner <brauner@kernel.org>
References: <20240412140122.2607743-1-stefanb@linux.ibm.com>
 <20240412140122.2607743-3-stefanb@linux.ibm.com>
 <CAOQ4uxjDQO91cjA0sgyPStkwc_7+NxAOhyve94qUvXSM3ytk1g@mail.gmail.com>
From: Stefan Berger <stefanb@linux.ibm.com>
In-Reply-To: <CAOQ4uxjDQO91cjA0sgyPStkwc_7+NxAOhyve94qUvXSM3ytk1g@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit


On 4/12/24 14:08, Amir Goldstein wrote:
> On Fri, Apr 12, 2024 at 5:01 PM Stefan Berger <stefanb@linux.ibm.com> wrote:
>>
>> On a stacked filesystem, when one process opens the file holding a file's
>> data (e.g., on upper or lower layer on overlayfs) then issue a violation
>> when another process opens the file for reading on the top layer (overlay
>> layer on overlayfs). This then provides similar behavior to the existing
>> case where a violation is generated when one process opens a file for
>> writing and another one opens the same file for reading. On stacked
>> filesystem also search all the lower layers for relevant files opened for
>> writing and issue the violation if one is found.
>>
>> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
>> ---
>>   security/integrity/ima/ima_main.c | 27 ++++++++++++++++++++++-----
>>   1 file changed, 22 insertions(+), 5 deletions(-)
>>
>> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
>> index f04f43af651c..590dd9d5d99a 100644
>> --- a/security/integrity/ima/ima_main.c
>> +++ b/security/integrity/ima/ima_main.c
>> @@ -121,8 +121,11 @@ static void ima_rdwr_violation_check(struct file *file,
>>                                       const char **pathname,
>>                                       char *filename)
>>   {
>> +       struct inode *real_inode = d_real_inode(file_dentry(file));
>>          struct inode *inode = file_inode(file);
>> +       struct dentry *fd_dentry, *d;
>>          fmode_t mode = file->f_mode;
>> +       struct inode *fd_inode;
>>          bool send_tomtou = false, send_writers = false;
>>
>>          if (mode & FMODE_WRITE) {
>> @@ -134,11 +137,25 @@ static void ima_rdwr_violation_check(struct file *file,
>>                                                  &iint->atomic_flags))
>>                                  send_tomtou = true;
>>                  }
>> -       } else {
>> -               if (must_measure)
>> -                       set_bit(IMA_MUST_MEASURE, &iint->atomic_flags);
>> -               if (inode_is_open_for_write(inode) && must_measure)
>> -                       send_writers = true;
>> +       } else if (must_measure) {
>> +               set_bit(IMA_MUST_MEASURE, &iint->atomic_flags);
>> +
>> +               if (inode == real_inode) {
>> +                       if (inode_is_open_for_write(inode))
>> +                               send_writers = true;
>> +               } else {
>> +                       d = d_real(file_dentry(file), D_REAL_FILEDATA);
>> +                       do {
>> +                               fd_dentry = d;
>> +                               fd_inode = d_inode(fd_dentry);
>> +                               if (inode_is_open_for_write(fd_inode)) {
>> +                                       send_writers = true;
>> +                                       break;
>> +                               }
>> +                               /* next layer of stacked fs */
>> +                               d = d_real(fd_dentry, D_REAL_FILEDATA);
>> +                       } while (d != fd_dentry);
>> +               }
> 
> The idea of digging though ovl layers feels wrong to me.

I have a couple of test cases that expect violations to be logged. One 
test case has 2 overlay filesystems stacked on top of each other (lower 
= A, upper = B) and it passes those test cases when for example

- opening the file on lower on 'A' for writing
- opening the file on overlay layer on 'B' for reading

OR

- opening the file on overlay layer on 'A' (= lower layer of 'B') for 
writing
- opening the file on overlay layer on 'B' for reading


After causing a copy-up only the following test case causes a violation 
to be logged:

- opening the file on upper on 'B' for writing
- opening the file on overlay layer on 'B' for reading

No violation will the be logged for example for:

- opening the file on overlay layer on 'A' (= lower of 'B') for writing
- opening the file on overlay layer on 'B' for reading


> As Miklos is the designer of overlayfs and its vfs architecture,

I was hoping that this would be sufficiently generic to work with 
potential future stacked filesystems as well that would need to also 
provide support for D_REAL_FILEDATA.

> I am deferring the call about adding this interface to Miklos.
> 
> Thanks,
> Amir.
>