linux-ima-devel — Discussion list for problems or development.

Re: [Linux-ima-devel] [PATCH v6 1/6] libfs: define simple_read_iter_from_buffer

[Mimi Z. <zo...@li...>]

On Thu, 2017-08-17 at 12:42 +1000, James Morris wrote:
> On Wed, 16 Aug 2017, Mimi Zohar wrote:
> 
> > On Wed, 2017-08-16 at 08:35 +0200, Christoph Hellwig wrote:
> > > Looks good,
> > > 
> > > Reviewed-by: Christoph Hellwig <hc...@ls...>
> > 
> > Thank you for the reviewed-by's.
> > 
> > Up to now I haven't been removing the Changelog before sending James a
> > pull request.  Adding the dashes in the commit itself, only changes
> > how the patches are applied by others to their local branch, not to
> > what would be upstreamed.  Am I suppose to be removing the changelog
> > before sending a pull request?
> 
> I don't really understand this, but leave the changelog in?

Christoph had commented that the "---" should be before the Changelog,
not afterwards, so that git-am skips them.  The changelog is needed by
reviewers, but once upstreamed it isn't really needed any more.  Based
on Christph's response (offline), any information in the changelog
that is still useful, should move into the commit description itself
or code.

Mimi

Re: [Linux-ima-devel] [PATCH, RESEND 08/12] ima: added parser for RPM data type

[Roberto S. <rob...@hu...>]

On 8/10/2017 3:12 PM, Mimi Zohar wrote:
> On Wed, 2017-08-09 at 19:18 +0200, Roberto Sassu wrote:
>> On 8/9/2017 4:30 PM, Mimi Zohar wrote:
>>> On Wed, 2017-08-09 at 11:15 +0200, Roberto Sassu wrote:
>>>> On 8/2/2017 9:22 AM, James Morris wrote:
>>>>> On Tue, 1 Aug 2017, Roberto Sassu wrote:
>>>>>
>>>>>> On 8/1/2017 12:27 PM, Christoph Hellwig wrote:
>>>>>>> On Tue, Aug 01, 2017 at 12:20:36PM +0200, Roberto Sassu wrote:
>>>>>>>> This patch introduces a parser for RPM packages. It extracts the digests
>>>>>>>> from the RPMTAG_FILEDIGESTS header section and converts them to binary
>>>>>>>> data
>>>>>>>> before adding them to the hash table.
>>>>>>>>
>>>>>>>> The advantage of this data type is that verifiers can determine who
>>>>>>>> produced that data, as headers are signed by Linux distributions vendors.
>>>>>>>> RPM headers signatures can be provided as digest list metadata.
>>>>>>>
>>>>>>> Err, parsing arbitrary file formats has no business in the kernel.
>>>>>>
>>>>>> The benefit of this choice is that no actions are required for
>>>>>> Linux distribution vendors to support the solution I'm proposing,
>>>>>> because they already provide signed digest lists (RPM headers).
>>>>>>
>>>>>> Since the proof of loading a digest list is the digest of the
>>>>>> digest list (included in the list metadata), if RPM headers are
>>>>>> converted to a different format, remote attestation verifiers
>>>>>> cannot check the signature.
>>>>>>
>>>>>> If the concern is security, it would be possible to prevent unsigned
>>>>>> RPM headers from being parsed, if the PGP key type is upstreamed
>>>>>> (adding in CC key...@vg...).
>>>>>
>>>>> It's a security concern and also a layering violation, there should be no
>>>>> need to parse package file formats in the kernel.
>>>>
>>>> Parsing RPMs is not strictly necessary. Digests from the headers
>>>> can be extracted and written to a new file using the compact data
>>>> format (introduced with patch 7/12).
>>>>
>>>> At boot time, IMA measures this file before digests are uploaded to the
>>>> kernel. At this point, only files with unknown digest will be added
>>>> to the measurement list. At verification time, verifiers recreate the
>>>> measurement list by merging together the digests uploaded to the
>>>> kernel with the unknown digests. Then, they verify the obtained list.
>>>>
>>>> There are two ways to verify the digests: searching them in a reference
>>>> database, or checking a signature. With the 'ima-sig' measurement list
>>>> template, it is possible to verify signatures for each accessed file.
>>>> With this patch set, it is possible to verify the signature of
>>>> the file containing the digests uploaded to the kernel. If the data
>>>> format changes, the signature cannot be verified.
>>>>
>>>> To avoid this limitation, the parsers could be moved to a userspace
>>>> tool which then uploads the parsed digests to the kernel. IMA would
>>>> measure the original files. But, if the tool is compromised, it could
>>>> load digests not included in the parsed files. With the current solution
>>>> this problem does not arise because no changes can be done by userspace
>>>> applications to the uploaded data while digests are parsed by IMA.
>>>>
>>>> I could remove the RPM parser from the patch set for now.
>>>>
>>>> Is the remaining part of the patch set ok, and is the explanation of
>>>> what it does clear?
>>>
>>> From a trusted boot perspective, file measurements are added to the
>>> measurement list, before access to the file is given.  The measurement
>>> list contains ALL measurements, as defined by policy.  This patch set
>>> changes that meaning to be all measurements, as defined by policy,
>>> with the exception of those in a white list.
>>
>> The digest list is also measured, so the measurement list is complete.
>> Verifiers have to check the digest of digest lists. Otherwise, they
>> would get an unknown digest and conclude that the system being verified
>> has been compromised.
>
> Your proposal is basically a pre-approved "batched" measurement, of a
> set of known good measurements, without the corresponding list of
> measurements that this "batched" measurement represents.  Right?

Yes, correct.


> This pre-approved "batched" measurement represents not what has been
> accessed/executed on the system, but what potentially could be
> accessed/executed.  That's a major difference.
>
>> If you prefer, I could add a new policy rule option to avoid file
>> measurements if the digest is in the digest list.
>
> Huh?  Patch "ima: don't report measurements if digests are included in
> the loaded lists" is already doing this.

Since the content of the measurement list depends on the policy,
adding a new option would give a better understanding of what the
measurement list represents. But, I agree that this is redundant.


>>> Changing the fundamental meaning of the measurement list is not
>>> acceptable.  You could define a new securityfs file to differentiate
>>> between the full measurement list and this abbreviated one.  But
>>
>> There cannot be two measurement lists at the same time. Providing the
>> full measurement list (containing the digest of files being accessed)
>> implies that its integrity must be protected with PCR extends, making
>> the optimization done by this patch set useless.
>
> True, so you would be able to configure the system with one or the
> other type of list, not both.  At least there would be a clear
> understanding of what that list represents.
>
>>
>>> before making this sort of change, I would prefer to address the
>>> underlying problem - TPM peformance.
>>
>> Even if the TPM driver performance improves significantly (17 seconds
>> for 1000 extends), the boot time delay would be still noticeable
>> (8.5 seconds for normal boot + 24 seconds for 1400 PCR extends).
>
> Agreed, there is still room for more TPM improvements.  Just Nayna's
> one patch, without any other changes, brought the timing down from 53s
> for a 1000 extends to just 11s.  (The initial patch was Nack'ed, but
> we're working with the tpmdd and the TCG's device driver work group
> (DDWG).)
>
>> In my opinion, this patch set is useful without considering the
>> performance improvement: reduced size of measurement lists and
>> verification of digest list signatures, instead of file signatures,
>> where signatures are already provided by Linux distributions.
>
> Right, there's always a trade off.  My suggestion, assuming we go with
> this approach, would be to make that trade off clear by using
> different lists.

You mean to add a new kernel command line option to create new
securityfs files instead of the existing ones
(ascii_runtime_measurements, binary_runtime_measurements)? I would
prefer a solution that does not change the interfaces, otherwise
remote attestation agents have to be modified. They can handle
the new list type, as the data format didn't change.

Thanks

Roberto


>>> There are a couple of things that could be done to improve the TPM
>>> driver performance, itself.  Once all of these options have been
>>> pursued, we could then consider batching the measurements to the TPM,
>>> meaning that the measurement list would still contain all the file
>>> measurements, but instead of extending the TPM for each measurement, a
>>> batched hash - a hash of a group of file measurements - would be
>>> extended into the TPM.
>>
>> Probably, I didn't explain clearly that this patch set does not decrease
>> the security of IMA.
>>
>> Extending the PCR for a group of file measurements means that the system
>> can be compromised between two PCR extends without detection because
>> a malicious binary could alter IMA before the next extend.
>
> Currently, a measurement is added to the measurement list and then is
> used to extend the TPM, before returning to the caller.
>
> A performance improvement would still first add the measurement to the
> measurement list, but would then queue and wait for the measurement to
> extend the TPM, before returning to the caller.  In a multi threaded
> environment, the queued measurements could be "batched" - a hash of a
> set of hashes - to extend the TPM.
>
> The delay would be at most two times it takes to extend the TPM - one
> to complete an existing current "batched" extend and another new
> "batched" extend.
>
> The difficulty with this approach is identifying which measurements
> are included in which "batched" measurement.  This approach provides
> the same guarantees as previously.
>
> Before making the TPM performance problem an IMA issue and "fixing" it
> in IMA, I would prefer that the TPM performance be addressed directly.
>
> Mimi
>
>>
>> This patch set extends the PCR with the digest of digest lists, before
>> files are accessed. No actions happen before either the digest lists
>> have been measured or the file measurement is added to the measurement
>> list, if the file digest is not included in the digest list.
>
>

-- 
HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Bo PENG, Qiuen PENG, Shengli WANG

Re: [Linux-ima-devel] [PATCH 11/12] ima: don't report measurements if digests are included in the loaded lists

[Roberto S. <rob...@hu...>]

On 8/9/2017 10:36 PM, Ken Goldman wrote:
> On 7/25/2017 11:44 AM, Roberto Sassu wrote:
>> Don't report measurements if the file digest has been included in
>> an uploaded digest list.
>>
>> The advantage of this solution is that the boot time overhead, when
>> a TPM is available, is very small because a PCR is extended only
>> for unknown files. The disadvantage is that verifiers do not know
>> anymore which and when files are accessed (they must assume that
>> the worst case happened, i.e. all files have been accessed).
>
> Am I reading this correctly that you want to measure certain files, but
> not ones that have been included in a "digest list", which sounds like a
> white list of sorts.
>
> If so, I have two concerns:
>
> 1 - How would the client get this digest list?  Shouldn't it be up to
> the relying party to decide what is trusted and not trusted, not the
> client?

The client can get the digest list from the RPM database or the
measurement list of a previous boot. A verifier still decides if the
client is trusted or not, depending on what could be possibly accessed
rather than what has been accessed.


> What of the case with two different relying parties that have a
> different list of trusted applications?  E.g., one trusts any version of
> program X, while the other trusts only version 3.1 and up?

There are two ways to provide more accurate information (make the list
of possibly accessed files closer to the list of accessed files):

1) use the measurement list of a previous boot as a source for
    the digest list

2) use the RPM database as a source for the digest lists, and load the
    headers of the latest version of RPMs

    and:

    load only the headers of RPMs including files that appear in the
    measurement list of a previous boot (this solution should be
    preferred to solution 1, as signature-based attestation can be done)


> 2 - What about files on the digest list that were not run?  The relying
> party may want to know if a program wasn't run?  E.g., antivirus or a
> firewall.
>
> If the rule is "don't measure if it's on the digest list", how does the
> relying party know if it was run?

If a measurement list does not contain unknown digests, this means that
no malicious software was loaded. But, if the measurement list contains
the digest of an antivirus binary, this does not necessarily mean that
it was loaded.

Since PCR 10 can be extended from userspace, a malicious user can
extend the PCR with the digest of a fake measurement entry reporting
the loading of the antivirus. He can then add the fake entry to the
measurement list before it is sent to verifiers, so that they won't
notice the addition.

A possible countermeasure to this attack would be to modify the TPM
driver to refuse to extend the PCRs used by IMA.

Then, IMA could be further extended to accept a list of digests of files
that will be certainly used and to create a measurement entry only after
all files have been accessed.

Roberto


> --
> To unsubscribe from this list: send the line "unsubscribe
> linux-security-module" in
> the body of a message to maj...@vg...
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Bo PENG, Qiuen PENG, Shengli WANG

Re: [Linux-ima-devel] [PATCH v6 1/6] libfs: define simple_read_iter_from_buffer

[James M. <jm...@na...>]

On Wed, 16 Aug 2017, Mimi Zohar wrote:

> On Wed, 2017-08-16 at 08:35 +0200, Christoph Hellwig wrote:
> > Looks good,
> > 
> > Reviewed-by: Christoph Hellwig <hc...@ls...>
> 
> Thank you for the reviewed-by's.
> 
> Up to now I haven't been removing the Changelog before sending James a
> pull request.  Adding the dashes in the commit itself, only changes
> how the patches are applied by others to their local branch, not to
> what would be upstreamed.  Am I suppose to be removing the changelog
> before sending a pull request?

I don't really understand this, but leave the changelog in?


-- 
James Morris
<jm...@na...>

Re: [Linux-ima-devel] [RFC PATCH 2/4] ima: define new ima_sb_post_new_mount hook

[James M. <jm...@na...>]

On Wed, 16 Aug 2017, Mimi Zohar wrote:

> In this context, I'm not sure what you mean by "loaded".  IMA needs to
> be enabled from the very beginning to capture all measurements and
> verify the integrity of files, without any gaps.  At some point this
> would include other LSM policies.

I think it's better to keep IMA orthogonal to LSM for this reason.

The original motivation to implement IMA as a separate API was because LSM 
was at the time considered specific to access control mechanisms, although 
that is not the case now.

-- 
James Morris
<jm...@na...>

Re: [Linux-ima-devel] [RFC PATCH 2/4] ima: define new ima_sb_post_new_mount hook

[Mimi Z. <zo...@li...>]

On Wed, 2017-08-16 at 12:24 -0700, Casey Schaufler wrote:
> On 8/16/2017 10:30 AM, Mimi Zohar wrote:

> > diff --git a/security/security.c b/security/security.c
> > index 592153e8d2b6..79111141b383 100644
> > --- a/security/security.c
> > +++ b/security/security.c
> > @@ -402,6 +402,7 @@ void security_sb_post_new_mount(const struct vfsmount *newmnt,
> >  				const struct path *path)
> >  {
> >  	call_void_hook(sb_post_new_mount, newmnt, path);
> > +	ima_sb_post_new_mount(newmnt, path);
> 
> Have you thought about converting the IMA code into
> a security module that gets loaded after all the others?
> That would make this (and several other similar instances)
> bit of special case code go away.

Yes, each time I need to add another IMA call, where there isn't an
existing LSM hook, a decision needs to be made as to whether it needs
to be a generic LSM hook or not.

Assuming we made IMA an LSM module, what would we do with these other
calls?  Would they need to be converted to LSM hooks?  (Are all LSMs
visited, even if an earlier LSM fails?  Or does the first LSM failure,
stop the LSM traversal?)

Unlike LSMs which are sharing the i_sec, IMA doesn't have an entry in
the inode, but does an rbtree lookup to access the associated data.
 Having an i_sec would simplify a lot of the code, but making this
sort of change would be a major undertaking.

In this context, I'm not sure what you mean by "loaded".  IMA needs to
be enabled from the very beginning to capture all measurements and
verify the integrity of files, without any gaps.  At some point this
would include other LSM policies.  IMA certainly cannot be loaded late
like kernel modules.

Similarly, we would need to think about EVM.

Mimi

Re: [Linux-ima-devel] [tpmdd-devel] [PATCH] tpm: improve tpm_tis send() performance by ignoring burstcount

[Ken G. <kg...@li...>]

On 8/14/2017 6:10 AM, Jarkko Sakkinen wrote:
>> Nuvoton, ST Micro, and Infineon confirmed that the TPM empties a byte from
>> the FIFO in under 1 usec.  So, even with a static burst count, the entire
>> FIFO would empty in under 10 usec.
> 
> Does this break anything lets say in a decade time frame? If it does, I
> will not even consider this. Can you give a definitive answer for that?

My attempt at predicting the future ...

1 - Clock speed should get faster, not slower, so the 1 usec wait state 
should get shorter.

2 - TPM buffers should get larger.  I'm already reading of 256 byte 
buffers.  So there should be fewer wait states.

3 - There is a trend toward using the CRB, making the FIFO less applicable.

4 - There is a trend away from LPC connected serial port devices, 
printers, and PS/2 mouse and keyboard, and toward USB connected devices. 
  I assume this will continue, making the already insignificant wait 
states irrelevant.

[Linux-ima-devel] [integrity:next-log-iversion 10/11] fs//attr.c:35:1: error: expected '=', ',', '; ', 'asm' or '__attribute__' before '{' token

[kbuild t. r. <fen...@in...>]

Hi Mimi,

First bad commit (maybe != root cause):

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/zohar/linux-integrity.git next-log-iversion
head:   5bb9c12032ed1b6bfc148cd8bf24e64996793553
commit: 6acf0ef6e57fb7f665049dbde14d9932826ba46c [10/11] security: define a new LSM sb_post_remount hook
config: i386-tinyconfig (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
        git checkout 6acf0ef6e57fb7f665049dbde14d9932826ba46c
        # save the attached .config to linux build tree
        make ARCH=i386 

All errors (new ones prefixed by >>):

    ^~~~~~~~
   In file included from include/linux/kexec.h:17:0,
                    from include/linux/ima.h:14,
                    from fs//attr.c:18:
   include/linux/crash_core.h:60:13: error: storage class specified for parameter 'vmcoreinfo_note'
    extern u32 *vmcoreinfo_note;
                ^~~~~~~~~~~~~~~
   In file included from include/uapi/linux/elfcore.h:8:0,
                    from include/linux/elfcore.h:9,
                    from include/linux/crash_core.h:5,
                    from include/linux/kexec.h:17,
                    from include/linux/ima.h:14,
                    from fs//attr.c:18:
   include/linux/elf.h:32:18: error: expected declaration specifiers before 'Elf32_Word'
    #define Elf_Word Elf32_Word
                     ^
   include/linux/crash_core.h:62:1: note: in expansion of macro 'Elf_Word'
    Elf_Word *append_elf_note(Elf_Word *buf, char *name, unsigned int type,
    ^~~~~~~~
   include/linux/elf.h:32:18: error: expected declaration specifiers or '...' before 'Elf32_Word'
    #define Elf_Word Elf32_Word
                     ^
   include/linux/crash_core.h:64:17: note: in expansion of macro 'Elf_Word'
    void final_note(Elf_Word *buf);
                    ^~~~~~~~
   In file included from include/linux/kexec.h:17:0,
                    from include/linux/ima.h:14,
                    from fs//attr.c:18:
   include/linux/crash_core.h:66:12: error: section attribute not allowed for 'parse_crashkernel'
    int __init parse_crashkernel(char *cmdline, unsigned long long system_ram,
               ^~~~~~~~~~~~~~~~~
   include/linux/crash_core.h:67:3: warning: '__cold__' attribute ignored [-Wattributes]
      unsigned long long *crash_size, unsigned long long *crash_base);
      ^~~~~~~~
   In file included from include/linux/ima.h:14:0,
                    from fs//attr.c:18:
   include/linux/kexec.h:331:1: warning: empty declaration
    struct pt_regs;
    ^~~~~~
   include/linux/kexec.h:332:1: warning: empty declaration
    struct task_struct;
    ^~~~~~
   include/linux/kexec.h:333:56: error: expected '=', ',', ';', 'asm' or '__attribute__' before '{' token
    static inline void __crash_kexec(struct pt_regs *regs) { }
                                                           ^
   include/linux/kexec.h:334:54: error: expected '=', ',', ';', 'asm' or '__attribute__' before '{' token
    static inline void crash_kexec(struct pt_regs *regs) { }
                                                         ^
   include/linux/kexec.h:335:61: error: expected '=', ',', ';', 'asm' or '__attribute__' before '{' token
    static inline int kexec_should_crash(struct task_struct *p) { return 0; }
                                                                ^
   include/linux/kexec.h:336:44: error: expected '=', ',', ';', 'asm' or '__attribute__' before '{' token
    static inline int kexec_crash_loaded(void) { return 0; }
                                               ^
   In file included from fs//attr.c:18:0:
   include/linux/ima.h:15:1: warning: empty declaration
    struct linux_binprm;
    ^~~~~~
   include/linux/ima.h:35:1: error: expected '=', ',', ';', 'asm' or '__attribute__' before '{' token
    {
    ^
   include/linux/ima.h:40:1: error: expected '=', ',', ';', 'asm' or '__attribute__' before '{' token
    {
    ^
   include/linux/ima.h:45:1: error: expected '=', ',', ';', 'asm' or '__attribute__' before '{' token
    {
    ^
   include/linux/ima.h:50:1: error: expected '=', ',', ';', 'asm' or '__attribute__' before '{' token
    {
    ^
   include/linux/ima.h:55:1: error: expected '=', ',', ';', 'asm' or '__attribute__' before '{' token
    {
    ^
   include/linux/ima.h:61:1: error: expected '=', ',', ';', 'asm' or '__attribute__' before '{' token
    {
    ^
   include/linux/ima.h:66:1: error: expected '=', ',', ';', 'asm' or '__attribute__' before '{' token
    {
    ^
   include/linux/ima.h:72:1: error: expected '=', ',', ';', 'asm' or '__attribute__' before '{' token
    { }
    ^
   include/linux/ima.h:76:1: warning: empty declaration
    struct kimage;
    ^~~~~~
   include/linux/ima.h:79:1: error: expected '=', ',', ';', 'asm' or '__attribute__' before '{' token
    {}
    ^
   include/linux/ima.h:90:1: error: expected '=', ',', ';', 'asm' or '__attribute__' before '{' token
    {
    ^
   include/linux/ima.h:95:1: error: expected '=', ',', ';', 'asm' or '__attribute__' before '{' token
    {
    ^
   include/linux/ima.h:103:1: error: expected '=', ',', ';', 'asm' or '__attribute__' before '{' token
    {
    ^
   include/linux/ima.h:109:1: error: expected '=', ',', ';', 'asm' or '__attribute__' before '{' token
    {
    ^
>> fs//attr.c:35:1: error: expected '=', ',', ';', 'asm' or '__attribute__' before '{' token
    {
    ^
>> fs//attr.c:96:31: error: expected declaration specifiers before ';' token
    EXPORT_SYMBOL(setattr_prepare);
                                  ^
   fs//attr.c:114:1: error: expected '=', ',', ';', 'asm' or '__attribute__' before '{' token
    {
    ^
   fs//attr.c:139:32: error: expected declaration specifiers before ';' token
    EXPORT_SYMBOL(inode_newsize_ok);
                                   ^
   fs//attr.c:157:1: error: expected '=', ',', ';', 'asm' or '__attribute__' before '{' token
    {
    ^
   fs//attr.c:182:28: error: expected declaration specifiers before ';' token
    EXPORT_SYMBOL(setattr_copy);
                               ^
   fs//attr.c:205:1: error: expected '=', ',', ';', 'asm' or '__attribute__' before '{' token
    {
    ^
   fs//attr.c:324:29: error: expected declaration specifiers before ';' token
    EXPORT_SYMBOL(notify_change);
                                ^
   In file included from fs//attr.c:16:0:
   include/linux/security.h:571:20: error: old-style parameter declarations in prototyped function definition
    static inline void security_sb_post_remount(const struct super_block *sb,
                       ^~~~~~~~~~~~~~~~~~~~~~~~
>> fs//attr.c:324:29: error: expected '{' at end of input
    EXPORT_SYMBOL(notify_change);
                                ^

vim +35 fs//attr.c

^1da177e Linus Torvalds    2005-04-16  19  
2c27c65e Christoph Hellwig 2010-06-04  20  /**
31051c85 Jan Kara          2016-05-26  21   * setattr_prepare - check if attribute changes to a dentry are allowed
31051c85 Jan Kara          2016-05-26  22   * @dentry:	dentry to check
2c27c65e Christoph Hellwig 2010-06-04  23   * @attr:	attributes to change
2c27c65e Christoph Hellwig 2010-06-04  24   *
2c27c65e Christoph Hellwig 2010-06-04  25   * Check if we are allowed to change the attributes contained in @attr
31051c85 Jan Kara          2016-05-26  26   * in the given dentry.  This includes the normal unix access permission
31051c85 Jan Kara          2016-05-26  27   * checks, as well as checks for rlimits and others. The function also clears
31051c85 Jan Kara          2016-05-26  28   * SGID bit from mode if user is not allowed to set it. Also file capabilities
31051c85 Jan Kara          2016-05-26  29   * and IMA extended attributes are cleared if ATTR_KILL_PRIV is set.
2c27c65e Christoph Hellwig 2010-06-04  30   *
2c27c65e Christoph Hellwig 2010-06-04  31   * Should be called as the first thing in ->setattr implementations,
2c27c65e Christoph Hellwig 2010-06-04  32   * possibly after taking additional locks.
2c27c65e Christoph Hellwig 2010-06-04  33   */
31051c85 Jan Kara          2016-05-26  34  int setattr_prepare(struct dentry *dentry, struct iattr *attr)
^1da177e Linus Torvalds    2005-04-16 @35  {
31051c85 Jan Kara          2016-05-26  36  	struct inode *inode = d_inode(dentry);
^1da177e Linus Torvalds    2005-04-16  37  	unsigned int ia_valid = attr->ia_valid;
^1da177e Linus Torvalds    2005-04-16  38  
2c27c65e Christoph Hellwig 2010-06-04  39  	/*
2c27c65e Christoph Hellwig 2010-06-04  40  	 * First check size constraints.  These can't be overriden using
2c27c65e Christoph Hellwig 2010-06-04  41  	 * ATTR_FORCE.
2c27c65e Christoph Hellwig 2010-06-04  42  	 */
2c27c65e Christoph Hellwig 2010-06-04  43  	if (ia_valid & ATTR_SIZE) {
2c27c65e Christoph Hellwig 2010-06-04  44  		int error = inode_newsize_ok(inode, attr->ia_size);
2c27c65e Christoph Hellwig 2010-06-04  45  		if (error)
2c27c65e Christoph Hellwig 2010-06-04  46  			return error;
2c27c65e Christoph Hellwig 2010-06-04  47  	}
2c27c65e Christoph Hellwig 2010-06-04  48  
^1da177e Linus Torvalds    2005-04-16  49  	/* If force is set do it anyway. */
^1da177e Linus Torvalds    2005-04-16  50  	if (ia_valid & ATTR_FORCE)
030b533c Jan Kara          2016-05-26  51  		goto kill_priv;
^1da177e Linus Torvalds    2005-04-16  52  
^1da177e Linus Torvalds    2005-04-16  53  	/* Make sure a caller can chown. */
^1da177e Linus Torvalds    2005-04-16  54  	if ((ia_valid & ATTR_UID) &&
8e96e3b7 Eric W. Biederman 2012-03-03  55  	    (!uid_eq(current_fsuid(), inode->i_uid) ||
7fa294c8 Eric W. Biederman 2012-09-02  56  	     !uid_eq(attr->ia_uid, inode->i_uid)) &&
23adbe12 Andy Lutomirski   2014-06-10  57  	    !capable_wrt_inode_uidgid(inode, CAP_CHOWN))
2c27c65e Christoph Hellwig 2010-06-04  58  		return -EPERM;
^1da177e Linus Torvalds    2005-04-16  59  
^1da177e Linus Torvalds    2005-04-16  60  	/* Make sure caller can chgrp. */
^1da177e Linus Torvalds    2005-04-16  61  	if ((ia_valid & ATTR_GID) &&
8e96e3b7 Eric W. Biederman 2012-03-03  62  	    (!uid_eq(current_fsuid(), inode->i_uid) ||
8e96e3b7 Eric W. Biederman 2012-03-03  63  	    (!in_group_p(attr->ia_gid) && !gid_eq(attr->ia_gid, inode->i_gid))) &&
23adbe12 Andy Lutomirski   2014-06-10  64  	    !capable_wrt_inode_uidgid(inode, CAP_CHOWN))
2c27c65e Christoph Hellwig 2010-06-04  65  		return -EPERM;
^1da177e Linus Torvalds    2005-04-16  66  
^1da177e Linus Torvalds    2005-04-16  67  	/* Make sure a caller can chmod. */
^1da177e Linus Torvalds    2005-04-16  68  	if (ia_valid & ATTR_MODE) {
2e149670 Serge E. Hallyn   2011-03-23  69  		if (!inode_owner_or_capable(inode))
2c27c65e Christoph Hellwig 2010-06-04  70  			return -EPERM;
^1da177e Linus Torvalds    2005-04-16  71  		/* Also check the setgid bit! */
^1da177e Linus Torvalds    2005-04-16  72  		if (!in_group_p((ia_valid & ATTR_GID) ? attr->ia_gid :
7fa294c8 Eric W. Biederman 2012-09-02  73  				inode->i_gid) &&
23adbe12 Andy Lutomirski   2014-06-10  74  		    !capable_wrt_inode_uidgid(inode, CAP_FSETID))
^1da177e Linus Torvalds    2005-04-16  75  			attr->ia_mode &= ~S_ISGID;
^1da177e Linus Torvalds    2005-04-16  76  	}
^1da177e Linus Torvalds    2005-04-16  77  
^1da177e Linus Torvalds    2005-04-16  78  	/* Check for setting the inode time. */
9767d749 Miklos Szeredi    2008-07-01  79  	if (ia_valid & (ATTR_MTIME_SET | ATTR_ATIME_SET | ATTR_TIMES_SET)) {
2e149670 Serge E. Hallyn   2011-03-23  80  		if (!inode_owner_or_capable(inode))
2c27c65e Christoph Hellwig 2010-06-04  81  			return -EPERM;
^1da177e Linus Torvalds    2005-04-16  82  	}
2c27c65e Christoph Hellwig 2010-06-04  83  
030b533c Jan Kara          2016-05-26  84  kill_priv:
030b533c Jan Kara          2016-05-26  85  	/* User has permission for the change */
030b533c Jan Kara          2016-05-26  86  	if (ia_valid & ATTR_KILL_PRIV) {
030b533c Jan Kara          2016-05-26  87  		int error;
030b533c Jan Kara          2016-05-26  88  
030b533c Jan Kara          2016-05-26  89  		error = security_inode_killpriv(dentry);
030b533c Jan Kara          2016-05-26  90  		if (error)
030b533c Jan Kara          2016-05-26  91  			return error;
030b533c Jan Kara          2016-05-26  92  	}
030b533c Jan Kara          2016-05-26  93  
2c27c65e Christoph Hellwig 2010-06-04  94  	return 0;
^1da177e Linus Torvalds    2005-04-16  95  }
31051c85 Jan Kara          2016-05-26 @96  EXPORT_SYMBOL(setattr_prepare);
^1da177e Linus Torvalds    2005-04-16  97  

:::::: The code at line 35 was first introduced by commit
:::::: 1da177e4c3f41524e886b7f1b8a0c1fc7321cac2 Linux-2.6.12-rc2

:::::: TO: Linus Torvalds <tor...@pp...>
:::::: CC: Linus Torvalds <tor...@pp...>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
-------------- next part --------------
A non-text attachment was scrubbed...
Name: .config.gz
Type: application/gzip
Size: 6670 bytes
Desc: not available

Re: [Linux-ima-devel] [RFC PATCH 2/4] ima: define new ima_sb_post_new_mount hook

[Casey S. <ca...@sc...>]

On 8/16/2017 10:30 AM, Mimi Zohar wrote:
> IMA measures a file, verifies a file's integrity, and caches the
> results.  On filesystems with MS_I_VERSION enabled, IMA can detect
> file changes and cause them to be re-measured and verified.  On
> filesystems without MS_I_VERSION enabled, files are measured and
> verified just once.
>
> This patch logs filesystems mounted without MS_I_VERSION.
>
> Signed-off-by: Mimi Zohar <zo...@li...>
> ---
>  include/linux/ima.h               |  5 +++++
>  security/integrity/ima/ima_main.c | 44 +++++++++++++++++++++++++++++++++++++++
>  security/security.c               |  1 +
>  3 files changed, 50 insertions(+)
>
> diff --git a/include/linux/ima.h b/include/linux/ima.h
> index 0e4647e0eb60..4475cb01149c 100644
> --- a/include/linux/ima.h
> +++ b/include/linux/ima.h
> @@ -23,6 +23,8 @@ extern int ima_read_file(struct file *file, enum kernel_read_file_id id);
>  extern int ima_post_read_file(struct file *file, void *buf, loff_t size,
>  			      enum kernel_read_file_id id);
>  extern void ima_post_path_mknod(struct dentry *dentry);
> +extern void ima_sb_post_new_mount(const struct vfsmount *newmnt,
> +				  const struct path *path);
>  
>  #ifdef CONFIG_IMA_KEXEC
>  extern void ima_add_kexec_buffer(struct kimage *image);
> @@ -65,6 +67,9 @@ static inline void ima_post_path_mknod(struct dentry *dentry)
>  	return;
>  }
>  
> +static inline void ima_sb_post_new_mount(const struct vfsmount *newmnt,
> +					 const struct path *path)
> +{ }
>  #endif /* CONFIG_IMA */
>  
>  #ifndef CONFIG_IMA_KEXEC
> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> index b00186914df8..a0a685189001 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -354,6 +354,50 @@ void ima_post_path_mknod(struct dentry *dentry)
>  }
>  
>  /**
> + * ima_sb_post_new_mount - check filesystem mounted flags
> + *
> + * Indicate that filesystem isn't mounted with i_version enabled.
> + */
> +void ima_sb_post_new_mount(const struct vfsmount *newmnt,
> +			   const struct path *path)
> +{
> +	struct super_block *sb;
> +	unsigned long pseudo_fs[] = {CGROUP_SUPER_MAGIC, CGROUP2_SUPER_MAGIC,
> +		SYSFS_MAGIC, DEVPTS_SUPER_MAGIC, PSTOREFS_MAGIC, EFIVARFS_MAGIC,
> +		DEBUGFS_MAGIC, TMPFS_MAGIC};
> +	char *pathbuf = NULL;
> +	char filename[NAME_MAX];
> +	const char *pathname;
> +	bool found = 0;
> +	int i;
> +
> +	sb = newmnt ? newmnt->mnt_sb : path->mnt->mnt_sb;
> +
> +	if ((sb->s_flags & MS_I_VERSION) || (sb->s_flags & MS_RDONLY) ||
> +	    (sb->s_flags & MS_KERNMOUNT))
> +		return;
> +
> +	for (i = 0; i < ARRAY_SIZE(pseudo_fs); i++) {
> +		if (pseudo_fs[i] != sb->s_magic)
> +			continue;
> +
> +		found = 1;
> +		break;
> +	}
> +	if (found)
> +		return;
> +
> +	pathname = ima_d_path(path, &pathbuf, filename);
> +	if (!pathname)
> +		return;
> +
> +	if (newmnt)
> +		pr_warn("ima: %s mounted without i_version enabled\n",
> +			pathname);
> +	 __putname(pathbuf);
> +}
> +
> +/**
>   * ima_read_file - pre-measure/appraise hook decision based on policy
>   * @file: pointer to the file to be measured/appraised/audit
>   * @read_id: caller identifier
> diff --git a/security/security.c b/security/security.c
> index 592153e8d2b6..79111141b383 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -402,6 +402,7 @@ void security_sb_post_new_mount(const struct vfsmount *newmnt,
>  				const struct path *path)
>  {
>  	call_void_hook(sb_post_new_mount, newmnt, path);
> +	ima_sb_post_new_mount(newmnt, path);

Have you thought about converting the IMA code into
a security module that gets loaded after all the others?
That would make this (and several other similar instances)
bit of special case code go away.


>  }
>  
>  int security_sb_umount(struct vfsmount *mnt, int flags)

Re: [Linux-ima-devel] [integrity:next-log-iversion 9/11] include/linux/ima.h:70:20: warning: 'ima_sb_post_new_mount' used but never defined

[Mimi Z. <zo...@li...>]

On Thu, 2017-08-17 at 01:58 +0800, kbuild test robot wrote:
> tree:   https://git.kernel.org/pub/scm/linux/kernel/git/zohar/linux-integrity.git next-log-iversion
> head:   5873cd8f629e0306acdc40d3a9142eb50ae70c2d
> commit: 58015ce0b48226300bb6805a75c99cf538a524c6 [9/11] ima: define new ima_sb_post_new_mount hook
> config: x86_64-randconfig-x007-201733 (attached as .config)
> compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
> reproduce:
>         git checkout 58015ce0b48226300bb6805a75c99cf538a524c6
>         # save the attached .config to linux build tree
>         make ARCH=x86_64 
> 
> All warnings (new ones prefixed by >>):
> 
>    In file included from security/security.c:22:0:
>    include/linux/ima.h:72:1: error: expected identifier or '(' before '{' token
>     { }
>     ^
>    In file included from security/security.c:22:0:
> >> include/linux/ima.h:70:20: warning: 'ima_sb_post_new_mount' used but never defined
>     static inline void ima_sb_post_new_mount(const struct vfsmount *newmnt,
>                        ^~~~~~~~~~~~~~~~~~~~~
> 
> vim +/ima_sb_post_new_mount +70 include/linux/ima.h
> 
>     69	
>   > 70	static inline void ima_sb_post_new_mount(const struct vfsmount *newmnt,
>     71						 const struct path *path);

It looks like the mistaken trailing semicolon cause this and the rest
of the errors.

Mimi

>   > 72	{ }
>     73	#endif /* CONFIG_IMA */
>     74	
> 

[Linux-ima-devel] [integrity:next-log-iversion 8/11] fs/namespace.c:2472:3: error: implicit declaration of function 'security_sb_post_new_mount'

[kbuild t. r. <fen...@in...>]

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/zohar/linux-integrity.git next-log-iversion
head:   5873cd8f629e0306acdc40d3a9142eb50ae70c2d
commit: 28449c1b5aea9dc632c13206cadd1087cd9a3f23 [8/11] security: define new LSM sb_post_new_mount hook
config: i386-tinyconfig (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
        git checkout 28449c1b5aea9dc632c13206cadd1087cd9a3f23
        # save the attached .config to linux build tree
        make ARCH=i386 

Note: the integrity/next-log-iversion HEAD 5873cd8f629e0306acdc40d3a9142eb50ae70c2d builds fine.
      It only hurts bisectibility.

All errors (new ones prefixed by >>):

   fs/namespace.c: In function 'do_new_mount':
>> fs/namespace.c:2472:3: error: implicit declaration of function 'security_sb_post_new_mount' [-Werror=implicit-function-declaration]
      security_sb_post_new_mount(mnt, path);
      ^~~~~~~~~~~~~~~~~~~~~~~~~~
   cc1: some warnings being treated as errors

vim +/security_sb_post_new_mount +2472 fs/namespace.c

  2435	
  2436	/*
  2437	 * create a new mount for userspace and request it to be added into the
  2438	 * namespace's tree
  2439	 */
  2440	static int do_new_mount(struct path *path, const char *fstype, int flags,
  2441				int mnt_flags, const char *name, void *data)
  2442	{
  2443		struct file_system_type *type;
  2444		struct vfsmount *mnt;
  2445		int err;
  2446	
  2447		if (!fstype)
  2448			return -EINVAL;
  2449	
  2450		type = get_fs_type(fstype);
  2451		if (!type)
  2452			return -ENODEV;
  2453	
  2454		mnt = vfs_kern_mount(type, flags, name, data);
  2455		if (!IS_ERR(mnt) && (type->fs_flags & FS_HAS_SUBTYPE) &&
  2456		    !mnt->mnt_sb->s_subtype)
  2457			mnt = fs_set_subtype(mnt, fstype);
  2458	
  2459		put_filesystem(type);
  2460		if (IS_ERR(mnt))
  2461			return PTR_ERR(mnt);
  2462	
  2463		if (mount_too_revealing(mnt, &mnt_flags)) {
  2464			mntput(mnt);
  2465			return -EPERM;
  2466		}
  2467	
  2468		err = do_add_mount(real_mount(mnt), path, mnt_flags);
  2469		if (err)
  2470			mntput(mnt);
  2471		else
> 2472			security_sb_post_new_mount(mnt, path);
  2473		return err;
  2474	}
  2475	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
-------------- next part --------------
A non-text attachment was scrubbed...
Name: .config.gz
Type: application/gzip
Size: 6670 bytes
Desc: not available

Re: [Linux-ima-devel] [PATCH v6 4/6] ima: use fs method to read integrity data

[Mimi Z. <zo...@li...>]

On Wed, 2017-08-16 at 15:17 +0200, Jan Kara wrote:
> On Tue 15-08-17 10:43:55, Mimi Zohar wrote:
> > From: Christoph Hellwig <hc...@ls...>
> > 
> > Add a new ->integrity_read file operation to read data for integrity
> > hash collection.  This is defined to be equivalent to ->read_iter,
> > except that it will be called with the i_rwsem held exclusively.
> > 
> > (Based on Christoph's original patch.)
> > 
> > Signed-off-by: Christoph Hellwig <hc...@ls...>
> > Cc: Matthew Garrett <mj...@sr...>
> > Cc: Jan Kara <ja...@su...>
> > Cc: "Theodore Ts'o" <ty...@mi...>
> > Cc: Andreas Dilger <adi...@di...>
> > Cc: Jaegeuk Kim <ja...@ke...>
> > Cc: Chao Yu <yu...@hu...>
> > Cc: Steven Whitehouse <swh...@re...>
> > Cc: Bob Peterson <rpe...@re...>
> > Cc: David Woodhouse <dw...@in...>
> > Cc: Dave Kleikamp <sh...@ke...>
> > Cc: Ryusuke Konishi <kon...@la...>
> > Cc: Mark Fasheh <mf...@ve...>
> > Cc: Joel Becker <jl...@ev...>
> > Cc: Richard Weinberger <ri...@no...>
> > Cc: "Darrick J. Wong" <dar...@or...>
> > Cc: Hugh Dickins <hu...@go...>
> > Cc: Chris Mason <cl...@fb...>
> > Signed-off-by: Mimi Zohar <zo...@li...>
> 
> Looks good to me. You can add:
> 
> Reviewed-by: Jan Kara <ja...@su...>

Thanks!

Re: [Linux-ima-devel] [PATCH v6 1/6] libfs: define simple_read_iter_from_buffer

[Mimi Z. <zo...@li...>]

On Wed, 2017-08-16 at 08:35 +0200, Christoph Hellwig wrote:
> Looks good,
> 
> Reviewed-by: Christoph Hellwig <hc...@ls...>

Thank you for the reviewed-by's.

Up to now I haven't been removing the Changelog before sending James a
pull request.  Adding the dashes in the commit itself, only changes
how the patches are applied by others to their local branch, not to
what would be upstreamed.  Am I suppose to be removing the changelog
before sending a pull request?

thanks,

Mimi

[Linux-ima-devel] [RFC PATCH 4/4] ima: define a new ima_sb_post_remount hook

[Mimi Z. <zo...@li...>]

Compare the previous i_version flag with the remounted i_version
flag.  Only if there is a change, log change message.

Signed-off-by: Mimi Zohar <zo...@li...>
---
 include/linux/ima.h               |  9 +++++++++
 security/integrity/ima/ima_main.c | 22 ++++++++++++++++++++--
 security/security.c               |  1 +
 3 files changed, 30 insertions(+), 2 deletions(-)

diff --git a/include/linux/ima.h b/include/linux/ima.h
index 4475cb01149c..bd98221c00d5 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -25,6 +25,9 @@ extern int ima_post_read_file(struct file *file, void *buf, loff_t size,
 extern void ima_post_path_mknod(struct dentry *dentry);
 extern void ima_sb_post_new_mount(const struct vfsmount *newmnt,
 				  const struct path *path);
+extern  void ima_sb_post_remount(const struct super_block *sb,
+				 unsigned long prev_sb_flags,
+				 const struct path *path);
 
 #ifdef CONFIG_IMA_KEXEC
 extern void ima_add_kexec_buffer(struct kimage *image);
@@ -70,6 +73,12 @@ static inline void ima_post_path_mknod(struct dentry *dentry)
 static inline void ima_sb_post_new_mount(const struct vfsmount *newmnt,
 					 const struct path *path)
 { }
+
+static inline void ima_sb_post_remount(const struct super_block *sb,
+				       unsigned long prev_sb_flags,
+				       const struct path *path)
+{ }
+
 #endif /* CONFIG_IMA */
 
 #ifndef CONFIG_IMA_KEXEC
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index a0a685189001..1b180f974e8d 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -373,8 +373,10 @@ void ima_sb_post_new_mount(const struct vfsmount *newmnt,
 
 	sb = newmnt ? newmnt->mnt_sb : path->mnt->mnt_sb;
 
-	if ((sb->s_flags & MS_I_VERSION) || (sb->s_flags & MS_RDONLY) ||
-	    (sb->s_flags & MS_KERNMOUNT))
+	if ((sb->s_flags & MS_KERNMOUNT) || (sb->s_flags & MS_RDONLY))
+		return;
+
+	if (newmnt && (sb->s_flags & MS_I_VERSION))
 		return;
 
 	for (i = 0; i < ARRAY_SIZE(pseudo_fs); i++) {
@@ -394,9 +396,25 @@ void ima_sb_post_new_mount(const struct vfsmount *newmnt,
 	if (newmnt)
 		pr_warn("ima: %s mounted without i_version enabled\n",
 			pathname);
+	else if (sb->s_flags & MS_I_VERSION)
+		pr_warn("ima: %s re-mounted with i_version enabled\n",
+			pathname);
+	else
+		pr_warn("ima: %s re-mounted without i_version enabled\n",
+			pathname);
 	 __putname(pathbuf);
 }
 
+void ima_sb_post_remount(const struct super_block *sb,
+			 unsigned long prev_sb_flags,
+			 const struct path *path)
+{
+	if ((sb->s_flags & MS_I_VERSION) == (prev_sb_flags & MS_I_VERSION))
+		return;		/* nothing changed */
+
+	ima_sb_post_new_mount(NULL, path);
+}
+
 /**
  * ima_read_file - pre-measure/appraise hook decision based on policy
  * @file: pointer to the file to be measured/appraised/audit
diff --git a/security/security.c b/security/security.c
index 7981ad9019c9..1e2051a52b9f 100644
--- a/security/security.c
+++ b/security/security.c
@@ -382,6 +382,7 @@ void security_sb_post_remount(const struct super_block *sb,
 			      const struct path *path)
 {
 	call_void_hook(sb_post_remount, sb, prev_sb_flags, path);
+	ima_sb_post_remount(sb, prev_sb_flags, path);
 }
 
 int security_sb_kern_mount(struct super_block *sb, int flags, void *data)
-- 
2.7.4

[Linux-ima-devel] [RFC PATCH 3/4] security: define a new LSM sb_post_remount hook

[Mimi Z. <zo...@li...>]

Allow LSMs to compare the previous and new mount flags. This patch
defines a new LSM hook named security_sb_post_remount.

Signed-off-by: Mimi Zohar <zo...@li...>
---
 fs/namespace.c            | 3 +++
 include/linux/lsm_hooks.h | 9 +++++++++
 include/linux/security.h  | 8 ++++++++
 security/security.c       | 7 +++++++
 4 files changed, 27 insertions(+)

diff --git a/fs/namespace.c b/fs/namespace.c
index a7fa13f422ad..e6f4ea9a1008 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -2228,6 +2228,7 @@ static int do_remount(struct path *path, int flags, int mnt_flags,
 	int err;
 	struct super_block *sb = path->mnt->mnt_sb;
 	struct mount *mnt = real_mount(path->mnt);
+	unsigned long prev_sb_flags = sb->s_flags;
 
 	if (!check_mnt(mnt))
 		return -EINVAL;
@@ -2279,6 +2280,8 @@ static int do_remount(struct path *path, int flags, int mnt_flags,
 		mnt->mnt.mnt_flags = mnt_flags;
 		touch_mnt_namespace(mnt->mnt_ns);
 		unlock_mount_hash();
+
+		security_sb_post_remount(sb, prev_sb_flags, path);
 	}
 	up_write(&sb->s_umount);
 	return err;
diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index c3ecea0d0dca..e940dc43786c 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -123,6 +123,11 @@
  *	@sb superblock being remounted
  *	@data contains the filesystem-specific data.
  *	Return 0 if permission is granted.
+ * @sb_post_remount:
+ *	Allow comparison between new and previous mount flags.
+ *	@sb superblock being remounted
+ *	@prev_sb_flags previous mount flags
+ *	@path contains the path for mount point object.
  * @sb_umount:
  *	Check permission before the @mnt file system is unmounted.
  *	@mnt contains the mounted file system.
@@ -1395,6 +1400,9 @@ union security_list_options {
 	void (*sb_free_security)(struct super_block *sb);
 	int (*sb_copy_data)(char *orig, char *copy);
 	int (*sb_remount)(struct super_block *sb, void *data);
+	void (*sb_post_remount)(const struct super_block *sb,
+				const unsigned long prev_sb_flags,
+				const struct path *path);
 	int (*sb_kern_mount)(struct super_block *sb, int flags, void *data);
 	int (*sb_show_options)(struct seq_file *m, struct super_block *sb);
 	int (*sb_statfs)(struct dentry *dentry);
@@ -1717,6 +1725,7 @@ struct security_hook_heads {
 	struct list_head sb_free_security;
 	struct list_head sb_copy_data;
 	struct list_head sb_remount;
+	struct list_head sb_post_remount;
 	struct list_head sb_kern_mount;
 	struct list_head sb_show_options;
 	struct list_head sb_statfs;
diff --git a/include/linux/security.h b/include/linux/security.h
index 4acdaae7aa04..fa963a870a84 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -237,6 +237,9 @@ int security_sb_alloc(struct super_block *sb);
 void security_sb_free(struct super_block *sb);
 int security_sb_copy_data(char *orig, char *copy);
 int security_sb_remount(struct super_block *sb, void *data);
+void security_sb_post_remount(const struct super_block *sb,
+			      unsigned long prev_sb_flags,
+			      const struct path *path);
 int security_sb_kern_mount(struct super_block *sb, int flags, void *data);
 int security_sb_show_options(struct seq_file *m, struct super_block *sb);
 int security_sb_statfs(struct dentry *dentry);
@@ -565,6 +568,11 @@ static inline int security_sb_remount(struct super_block *sb, void *data)
 	return 0;
 }
 
+static inline void security_sb_post_remount(const struct super_block *sb,
+					    unsigned long prev_sb_flags,
+					    const struct path *path))
+{ }
+
 static inline int security_sb_kern_mount(struct super_block *sb, int flags, void *data)
 {
 	return 0;
diff --git a/security/security.c b/security/security.c
index 79111141b383..7981ad9019c9 100644
--- a/security/security.c
+++ b/security/security.c
@@ -377,6 +377,13 @@ int security_sb_remount(struct super_block *sb, void *data)
 	return call_int_hook(sb_remount, 0, sb, data);
 }
 
+void security_sb_post_remount(const struct super_block *sb,
+			      unsigned long prev_sb_flags,
+			      const struct path *path)
+{
+	call_void_hook(sb_post_remount, sb, prev_sb_flags, path);
+}
+
 int security_sb_kern_mount(struct super_block *sb, int flags, void *data)
 {
 	return call_int_hook(sb_kern_mount, 0, sb, flags, data);
-- 
2.7.4

[Linux-ima-devel] [RFC PATCH 2/4] ima: define new ima_sb_post_new_mount hook

[Mimi Z. <zo...@li...>]

IMA measures a file, verifies a file's integrity, and caches the
results.  On filesystems with MS_I_VERSION enabled, IMA can detect
file changes and cause them to be re-measured and verified.  On
filesystems without MS_I_VERSION enabled, files are measured and
verified just once.

This patch logs filesystems mounted without MS_I_VERSION.

Signed-off-by: Mimi Zohar <zo...@li...>
---
 include/linux/ima.h               |  5 +++++
 security/integrity/ima/ima_main.c | 44 +++++++++++++++++++++++++++++++++++++++
 security/security.c               |  1 +
 3 files changed, 50 insertions(+)

diff --git a/include/linux/ima.h b/include/linux/ima.h
index 0e4647e0eb60..4475cb01149c 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -23,6 +23,8 @@ extern int ima_read_file(struct file *file, enum kernel_read_file_id id);
 extern int ima_post_read_file(struct file *file, void *buf, loff_t size,
 			      enum kernel_read_file_id id);
 extern void ima_post_path_mknod(struct dentry *dentry);
+extern void ima_sb_post_new_mount(const struct vfsmount *newmnt,
+				  const struct path *path);
 
 #ifdef CONFIG_IMA_KEXEC
 extern void ima_add_kexec_buffer(struct kimage *image);
@@ -65,6 +67,9 @@ static inline void ima_post_path_mknod(struct dentry *dentry)
 	return;
 }
 
+static inline void ima_sb_post_new_mount(const struct vfsmount *newmnt,
+					 const struct path *path)
+{ }
 #endif /* CONFIG_IMA */
 
 #ifndef CONFIG_IMA_KEXEC
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index b00186914df8..a0a685189001 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -354,6 +354,50 @@ void ima_post_path_mknod(struct dentry *dentry)
 }
 
 /**
+ * ima_sb_post_new_mount - check filesystem mounted flags
+ *
+ * Indicate that filesystem isn't mounted with i_version enabled.
+ */
+void ima_sb_post_new_mount(const struct vfsmount *newmnt,
+			   const struct path *path)
+{
+	struct super_block *sb;
+	unsigned long pseudo_fs[] = {CGROUP_SUPER_MAGIC, CGROUP2_SUPER_MAGIC,
+		SYSFS_MAGIC, DEVPTS_SUPER_MAGIC, PSTOREFS_MAGIC, EFIVARFS_MAGIC,
+		DEBUGFS_MAGIC, TMPFS_MAGIC};
+	char *pathbuf = NULL;
+	char filename[NAME_MAX];
+	const char *pathname;
+	bool found = 0;
+	int i;
+
+	sb = newmnt ? newmnt->mnt_sb : path->mnt->mnt_sb;
+
+	if ((sb->s_flags & MS_I_VERSION) || (sb->s_flags & MS_RDONLY) ||
+	    (sb->s_flags & MS_KERNMOUNT))
+		return;
+
+	for (i = 0; i < ARRAY_SIZE(pseudo_fs); i++) {
+		if (pseudo_fs[i] != sb->s_magic)
+			continue;
+
+		found = 1;
+		break;
+	}
+	if (found)
+		return;
+
+	pathname = ima_d_path(path, &pathbuf, filename);
+	if (!pathname)
+		return;
+
+	if (newmnt)
+		pr_warn("ima: %s mounted without i_version enabled\n",
+			pathname);
+	 __putname(pathbuf);
+}
+
+/**
  * ima_read_file - pre-measure/appraise hook decision based on policy
  * @file: pointer to the file to be measured/appraised/audit
  * @read_id: caller identifier
diff --git a/security/security.c b/security/security.c
index 592153e8d2b6..79111141b383 100644
--- a/security/security.c
+++ b/security/security.c
@@ -402,6 +402,7 @@ void security_sb_post_new_mount(const struct vfsmount *newmnt,
 				const struct path *path)
 {
 	call_void_hook(sb_post_new_mount, newmnt, path);
+	ima_sb_post_new_mount(newmnt, path);
 }
 
 int security_sb_umount(struct vfsmount *mnt, int flags)
-- 
2.7.4

[Linux-ima-devel] [RFC PATCH 1/4] security: define new LSM sb_post_new_mount hook

[Mimi Z. <zo...@li...>]

Different filesystems enable different flags automatically, while
others require the mount flag to be supplied as a mount option (eg.
i_version).  Although this hook is post mount, permit logging or
auditing missing flags.

Signed-off-by: Mimi Zohar <zo...@li...>
---
 fs/namespace.c            | 2 ++
 include/linux/lsm_hooks.h | 7 +++++++
 include/linux/security.h  | 2 ++
 security/security.c       | 6 ++++++
 4 files changed, 17 insertions(+)

diff --git a/fs/namespace.c b/fs/namespace.c
index f8893dc6a989..a7fa13f422ad 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -2468,6 +2468,8 @@ static int do_new_mount(struct path *path, const char *fstype, int flags,
 	err = do_add_mount(real_mount(mnt), path, mnt_flags);
 	if (err)
 		mntput(mnt);
+	else
+		security_sb_post_new_mount(mnt, path);
 	return err;
 }
 
diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index ce02f76a6188..c3ecea0d0dca 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -128,6 +128,10 @@
  *	@mnt contains the mounted file system.
  *	@flags contains the unmount flags, e.g. MNT_FORCE.
  *	Return 0 if permission is granted.
+ * @sb_post_new_mount:
+ *	Check mounted options conform to expectations
+ *	@newmnt contains the newly mounted file system.
+ *	@path contains the path for mount point object.
  * @sb_pivotroot:
  *	Check permission before pivoting the root filesystem.
  *	@old_path contains the path for the new location of the
@@ -1396,6 +1400,8 @@ union security_list_options {
 	int (*sb_statfs)(struct dentry *dentry);
 	int (*sb_mount)(const char *dev_name, const struct path *path,
 			const char *type, unsigned long flags, void *data);
+	void (*sb_post_new_mount)(const struct vfsmount *newmnt,
+				  const struct path *path);
 	int (*sb_umount)(struct vfsmount *mnt, int flags);
 	int (*sb_pivotroot)(const struct path *old_path, const struct path *new_path);
 	int (*sb_set_mnt_opts)(struct super_block *sb,
@@ -1716,6 +1722,7 @@ struct security_hook_heads {
 	struct list_head sb_statfs;
 	struct list_head sb_mount;
 	struct list_head sb_umount;
+	struct list_head sb_post_new_mount;
 	struct list_head sb_pivotroot;
 	struct list_head sb_set_mnt_opts;
 	struct list_head sb_clone_mnt_opts;
diff --git a/include/linux/security.h b/include/linux/security.h
index 458e24bea2d4..4acdaae7aa04 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -242,6 +242,8 @@ int security_sb_show_options(struct seq_file *m, struct super_block *sb);
 int security_sb_statfs(struct dentry *dentry);
 int security_sb_mount(const char *dev_name, const struct path *path,
 		      const char *type, unsigned long flags, void *data);
+void security_sb_post_new_mount(const struct vfsmount *mnt,
+				const struct path *path);
 int security_sb_umount(struct vfsmount *mnt, int flags);
 int security_sb_pivotroot(const struct path *old_path, const struct path *new_path);
 int security_sb_set_mnt_opts(struct super_block *sb,
diff --git a/security/security.c b/security/security.c
index 55b5997e4b72..592153e8d2b6 100644
--- a/security/security.c
+++ b/security/security.c
@@ -398,6 +398,12 @@ int security_sb_mount(const char *dev_name, const struct path *path,
 	return call_int_hook(sb_mount, 0, dev_name, path, type, flags, data);
 }
 
+void security_sb_post_new_mount(const struct vfsmount *newmnt,
+				const struct path *path)
+{
+	call_void_hook(sb_post_new_mount, newmnt, path);
+}
+
 int security_sb_umount(struct vfsmount *mnt, int flags)
 {
 	return call_int_hook(sb_umount, 0, mnt, flags);
-- 
2.7.4

[Linux-ima-devel] [RFC PATCH 0/4] ima: filesystems not mounted with i_version

[Mimi Z. <zo...@li...>]

IMA measures a file, verifies a file's integrity, and caches the
results.  On filesystems with MS_I_VERSION enabled, IMA can detect
file changes and cause them to be re-measured and verified.  On
filesystems without MS_I_VERSION enabled, files are measured and
verified just once.

Currently users either have to look at the source code or test 
to determine if the file system supports i_version.  Even if the
file system supports i_version, there is no guarantee that the
filesystem was actually mounted with the i_version flag.

This patch set emits warning messages when filesystems are not
mounted with i_version support.  This patch set defines two new post
LSM hooks named security_sb_post_new_mount and
security_sb_post_remount, with their corresponding IMA functions.

Questions:
- IMA can call out directly to the IMA functions, without having to
  define these LSM hooks.  Is there a need for these LSM hooks?

- do_new_mount() creates a new vfsmount. If there is a way of accessing
  this new vfsmount from the caller do_mount(), we would only need one
  new LSM hook and corresponding IMA hook.

Mimi

Mimi Zohar (4):
  security: define new LSM sb_post_new_mount hook
  ima: define new ima_sb_post_new_mount hook
  security: define a new LSM sb_post_remount hook
  ima: define a new ima_sb_post_remount hook

 fs/namespace.c                    |  5 ++++
 include/linux/ima.h               | 14 +++++++++
 include/linux/lsm_hooks.h         | 16 ++++++++++
 include/linux/security.h          | 10 +++++++
 security/integrity/ima/ima_main.c | 62 +++++++++++++++++++++++++++++++++++++++
 security/security.c               | 15 ++++++++++
 6 files changed, 122 insertions(+)

-- 
2.7.4

[Linux-ima-devel] [integrity:next-log-iversion 9/11] include/linux/ima.h:72:1: error: expected identifier or '(' before '{' token

[kbuild t. r. <fen...@in...>]

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/zohar/linux-integrity.git next-log-iversion
head:   5873cd8f629e0306acdc40d3a9142eb50ae70c2d
commit: 58015ce0b48226300bb6805a75c99cf538a524c6 [9/11] ima: define new ima_sb_post_new_mount hook
config: i386-tinyconfig (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
        git checkout 58015ce0b48226300bb6805a75c99cf538a524c6
        # save the attached .config to linux build tree
        make ARCH=i386 

Note: the integrity/next-log-iversion HEAD 5873cd8f629e0306acdc40d3a9142eb50ae70c2d builds fine.
      It only hurts bisectibility.

All errors (new ones prefixed by >>):

   In file included from fs//attr.c:18:0:
>> include/linux/ima.h:72:1: error: expected identifier or '(' before '{' token
    { }
    ^
   include/linux/ima.h:70:20: warning: 'ima_sb_post_new_mount' declared 'static' but never defined [-Wunused-function]
    static inline void ima_sb_post_new_mount(const struct vfsmount *newmnt,
                       ^~~~~~~~~~~~~~~~~~~~~

vim +72 include/linux/ima.h

    69	
    70	static inline void ima_sb_post_new_mount(const struct vfsmount *newmnt,
    71						 const struct path *path);
  > 72	{ }
    73	#endif /* CONFIG_IMA */
    74	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
-------------- next part --------------
A non-text attachment was scrubbed...
Name: .config.gz
Type: application/gzip
Size: 6670 bytes
Desc: not available

Re: [Linux-ima-devel] [PATCH v6 4/6] ima: use fs method to read integrity data

[Jan K. <ja...@su...>]

On Tue 15-08-17 10:43:55, Mimi Zohar wrote:
> From: Christoph Hellwig <hc...@ls...>
> 
> Add a new ->integrity_read file operation to read data for integrity
> hash collection.  This is defined to be equivalent to ->read_iter,
> except that it will be called with the i_rwsem held exclusively.
> 
> (Based on Christoph's original patch.)
> 
> Signed-off-by: Christoph Hellwig <hc...@ls...>
> Cc: Matthew Garrett <mj...@sr...>
> Cc: Jan Kara <ja...@su...>
> Cc: "Theodore Ts'o" <ty...@mi...>
> Cc: Andreas Dilger <adi...@di...>
> Cc: Jaegeuk Kim <ja...@ke...>
> Cc: Chao Yu <yu...@hu...>
> Cc: Steven Whitehouse <swh...@re...>
> Cc: Bob Peterson <rpe...@re...>
> Cc: David Woodhouse <dw...@in...>
> Cc: Dave Kleikamp <sh...@ke...>
> Cc: Ryusuke Konishi <kon...@la...>
> Cc: Mark Fasheh <mf...@ve...>
> Cc: Joel Becker <jl...@ev...>
> Cc: Richard Weinberger <ri...@no...>
> Cc: "Darrick J. Wong" <dar...@or...>
> Cc: Hugh Dickins <hu...@go...>
> Cc: Chris Mason <cl...@fb...>
> Signed-off-by: Mimi Zohar <zo...@li...>

Looks good to me. You can add:

Reviewed-by: Jan Kara <ja...@su...>

								Honza
-- 
Jan Kara <ja...@su...>
SUSE Labs, CR

Re: [Linux-ima-devel] [PATCH v6 0/6] define new fs integrity_read method

[Mimi Z. <zo...@li...>]

On Wed, 2017-08-16 at 19:52 +1000, James Morris wrote:
> On Wed, 16 Aug 2017, Christoph Hellwig wrote:
> 
> > On Wed, Aug 16, 2017 at 12:43:58PM +1000, James Morris wrote:
> > > On Tue, 15 Aug 2017, Mimi Zohar wrote:
> > > 
> > > > To resolve this locking problem, this patch set introduces a new
> > > > ->integrity_read file operation method.  Until all filesystems
> > > > define the new ->integrity_read method, files that were previously
> > > > measured might not be currently measured and files that were
> > > > previously appraised might fail to be appraised properly.
> > > 
> > > Are there any such filesystems in mainline which are not getting an 
> > > integrity_read method in this patchset?
> > 
> > There are a few, mostly because we're pretty sure the previous integrity
> > code did the wrong thing for them - e.g. ocfs2 and gfs2 where locking
> > vs operations on other cluster nodes was missing, or NFS where in addition
> > to the above deadlocks were 100% reprodicible with current code.
> 
> Should we do a warn_once for these filesystems when IMA is used?

I don't think it is necessary.  In terms of IMA-measurement, any file
in policy on an unsupported filesystem will be in the measurement
list, but the file hash will be 0's.  In terms of IMA-appraisal, any
file in policy on an unsupported filesystem will fail appraisal, since
the file hash is 0.

A separate patch set will emit a warning when a file system is mounted
without i_version.

Mimi

Re: [Linux-ima-devel] [tpmdd-devel] [PATCH] tpm: improve tpm_tis send() performance by ignoring burstcount

[Michal S. <msu...@su...>]

On Tue, 15 Aug 2017 18:02:57 -0400
Ken Goldman <kg...@li...> wrote:

> On 8/13/2017 7:53 PM, msuchanek wrote:
> > About 500 out of 700 mainboards sold today has a PS/2 port which is
> > probably due to prevalence of legacy devices and usbhid limitations.
> > 
> > Similarily many boards have serial and parallel hardware ports.
> > 
> > In all diagrams detailed enough to show these ports I have seen them
> > attached to the LPC bus.  
> 
> Do these boards have a TPM?  Remember that the TPM requires special
> LPC bus cycles.

Out of nearly 700 boards over 500 have PS/2 connector and over 400
have TPM slot (which is subset of the PS/2 enabled boards). Some more
possibly have on-board TPM chip.

> 
> Even if so, the TPM LPC bus wait states are less than a usec.  My 
> thought is that  it's unlikely that any device (serial port, mouse, 
> keyboard, printer) will be adversely affected.

Yes, in theory this is negligible. So unless there is a possibility
these wait states chain or the device otherwise takes over the bus for
extended period of time this should be fine.

Thanks

Michal

Re: [Linux-ima-devel] [PATCH v6 0/6] define new fs integrity_read method

[James M. <jm...@na...>]

On Wed, 16 Aug 2017, Christoph Hellwig wrote:

> On Wed, Aug 16, 2017 at 12:43:58PM +1000, James Morris wrote:
> > On Tue, 15 Aug 2017, Mimi Zohar wrote:
> > 
> > > To resolve this locking problem, this patch set introduces a new
> > > ->integrity_read file operation method.  Until all filesystems
> > > define the new ->integrity_read method, files that were previously
> > > measured might not be currently measured and files that were
> > > previously appraised might fail to be appraised properly.
> > 
> > Are there any such filesystems in mainline which are not getting an 
> > integrity_read method in this patchset?
> 
> There are a few, mostly because we're pretty sure the previous integrity
> code did the wrong thing for them - e.g. ocfs2 and gfs2 where locking
> vs operations on other cluster nodes was missing, or NFS where in addition
> to the above deadlocks were 100% reprodicible with current code.

Should we do a warn_once for these filesystems when IMA is used?


-- 
James Morris
<jm...@na...>

Re: [Linux-ima-devel] [PATCH v6 2/6] efivarfs: replaces the read file operation with read_iter

[Christoph H. <hc...@ls...>]

Looks good,

Reviewed-by: Christoph Hellwig <hc...@ls...>

Re: [Linux-ima-devel] [PATCH v6 1/6] libfs: define simple_read_iter_from_buffer

[Christoph H. <hc...@ls...>]

Looks good,

Reviewed-by: Christoph Hellwig <hc...@ls...>