Re: [PATCH 0/2] ima: policy search speedup
On Wed, Dec 12, 2012 at 8:56 AM, Kasatkin, Dmitry wrote: > I have done few tests. > Ratio between lookups is different, but I do not really remember what > exactly it was. > Probably I did measurement with directory integrity protection... > > First test is done using upstream IMA. > > IMA-appraisal > [ 59.554072] counter1 - before (inode->i_flags & S_NOIMA): 74586 > [ 59.554628] counter2 - after (inode->i_flags & S_NOIMA): 74558 > [ 59.555024] counter3 - after (inode->i_sb->s_feature_flags & SF_NOIMA): > 52895 > > You can see 3 counters after 55 seconds uptime. > Counters count entries to the policy search function. > First counter counts every entry. > Second counter counts if control passes after inode flag S_NOIMA. > Third counts if control passes after SB flag SF_NOIMA. > > As you can see, inode flag does not make a difference - just 28 > entries differences. That is quite shocking to me that on system start up we only ever reference the same /proc file 28 times. Wow. I just don't know what to say. Obviously the per inode flag is dead. Now there is probably some CPU benefit to a per-sb flag, but I'm not sure it is worth the code to do it if you can't really measure that as something that makes a difference in userspace. Maybe if you can show that after your integrity protection work is done we can revisit. But I'd think this patch is dead until you can show the impact on real userspace. Someone else might disagree, but that's my thought. Sorry. -Eric -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/2] ima: policy search speedup
On Wed, Dec 12, 2012 at 1:02 AM, Eric Paris wrote: > Linus made it clear he likes per-inode. Do a test with per-inode > S_NOIMA. See how that compares to your 100,000 vs 10,000 lookups. > Then we can discuss with facts if per sb is worth it or not. We still > don't actually know if 100,000 lookups vs 10,000 lookups really > matters, but at least we'll have some facts... > Hello, I have done few tests. Ratio between lookups is different, but I do not really remember what exactly it was. Probably I did measurement with directory integrity protection... First test is done using upstream IMA. IMA-appraisal [ 59.554072] counter1 - before (inode->i_flags & S_NOIMA): 74586 [ 59.554628] counter2 - after (inode->i_flags & S_NOIMA): 74558 [ 59.555024] counter3 - after (inode->i_sb->s_feature_flags & SF_NOIMA): 52895 You can see 3 counters after 55 seconds uptime. Counters count entries to the policy search function. First counter counts every entry. Second counter counts if control passes after inode flag S_NOIMA. Third counts if control passes after SB flag SF_NOIMA. As you can see, inode flag does not make a difference - just 28 entries differences. But for SB flag, difference is over 22000. Directory integrity protection is not really relevant to this discussion, because it is under work now, but with directory integrity protection ratio is very different. IMA-appraisal with directories [ 59.999077] counter1 - before (inode->i_flags & S_NOIMA): 198761 [ 59.999078] counter2 - after (inode->i_flags & S_NOIMA): 198728 [ 59.999079] counter3 - after (inode->i_sb->s_feature_flags & SF_NOIMA): 50266 Again, inode flags does not make a difference. SB flag gives ~150 000 less searches. Basically what I want to show is that inode flag has no benefit. We need SB flag for that. Regarding referencing i_sb, IMA policy search might do it for every rule, like if ((rule->flags & IMA_FSMAGIC) && rule->fsmagic != inode->i_sb->s_magic) return false; So 20k for 55 seconds can be multiplied roughly by the number of rules. In fact earlier check for (inode->i_sb->s_feature_flags & SF_NOIMA) only decreases the total number of referencing. - Dmitry > On Tue, Dec 11, 2012 at 5:57 PM, Kasatkin, Dmitry > wrote: >> On Tue, Dec 11, 2012 at 10:08 PM, Eric Paris wrote: >>> S_PRIVATE is totally unacceptable as it has a meaning across all LSMs, >>> not just IMA. >>> >>> S_NOSEC means 'this is not setuid or setgid and we don't need to do >>> those checks on modify' >>> >>> You are going to need to use a S_NOIMA. >>> >>> Of Dmitry's 90,000 fewer policy lookups using the per sb flag, how >>> many of them are the same inode over and over again which would be >>> circumvented using S_NOIMA per inode flag? >>> >> >> IIRC those were only newly instantiated inodes. >> >> For new inodes, S_NOIMA would not be set and used at that point. >> IMA must do the policy search and then would set the S_NOIMA. >> >> For subsequent calls, S_NOIMA makes sense. >> >> If we would have the SB flag like MS_NOIMA, then we could replicate sb >> flag to inode S_NOIMA flag, >> in similar way as inode_has_no_xattr() does: >> >> static inline void inode_has_no_xattr(struct inode *inode) >> { >> if (!is_sxid(inode->i_mode) && (inode->i_sb->s_flags & MS_NOSEC)) >> inode->i_flags |= S_NOSEC; >> } >> >> Then later there is no need to dereference inode->i_sb... >> >> Can we reach conclusion about it? >> Will we have SB flag? >> if yes, then where, s_flags, or in some other field like s_feature_flags? >> if not, then we can stop this discussion... >> >> - Dmitry >> >>> >>> >>> On Tue, Dec 11, 2012 at 2:48 PM, Mimi Zohar >>> wrote: On Tue, 2012-12-11 at 11:10 -0800, Linus Torvalds wrote: > Anyway, the whole "you can do it at file granularity" isn't the bulk > of my argument (the "we already have the field that makes sense" is). > But my point is that per-inode is not only the logically more > straightforward place to do it, it's also the much more flexible place > to do it. Because it *allows* for things like that. Ok. To summarize, S_IMA indicates that there is a rule and that the iint was allocated. To differentiate between 'haven't looked/don't know' and 'definitely not', we need another bit. For this, you're suggesting using IS_PRIVATE()? Hopefully, I misunderstood. thanks, Mimi -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/2] ima: policy search speedup
Linus made it clear he likes per-inode. Do a test with per-inode S_NOIMA. See how that compares to your 100,000 vs 10,000 lookups. Then we can discuss with facts if per sb is worth it or not. We still don't actually know if 100,000 lookups vs 10,000 lookups really matters, but at least we'll have some facts... On Tue, Dec 11, 2012 at 5:57 PM, Kasatkin, Dmitry wrote: > On Tue, Dec 11, 2012 at 10:08 PM, Eric Paris wrote: >> S_PRIVATE is totally unacceptable as it has a meaning across all LSMs, >> not just IMA. >> >> S_NOSEC means 'this is not setuid or setgid and we don't need to do >> those checks on modify' >> >> You are going to need to use a S_NOIMA. >> >> Of Dmitry's 90,000 fewer policy lookups using the per sb flag, how >> many of them are the same inode over and over again which would be >> circumvented using S_NOIMA per inode flag? >> > > IIRC those were only newly instantiated inodes. > > For new inodes, S_NOIMA would not be set and used at that point. > IMA must do the policy search and then would set the S_NOIMA. > > For subsequent calls, S_NOIMA makes sense. > > If we would have the SB flag like MS_NOIMA, then we could replicate sb > flag to inode S_NOIMA flag, > in similar way as inode_has_no_xattr() does: > > static inline void inode_has_no_xattr(struct inode *inode) > { > if (!is_sxid(inode->i_mode) && (inode->i_sb->s_flags & MS_NOSEC)) > inode->i_flags |= S_NOSEC; > } > > Then later there is no need to dereference inode->i_sb... > > Can we reach conclusion about it? > Will we have SB flag? > if yes, then where, s_flags, or in some other field like s_feature_flags? > if not, then we can stop this discussion... > > - Dmitry > >> >> >> On Tue, Dec 11, 2012 at 2:48 PM, Mimi Zohar wrote: >>> On Tue, 2012-12-11 at 11:10 -0800, Linus Torvalds wrote: >>> Anyway, the whole "you can do it at file granularity" isn't the bulk of my argument (the "we already have the field that makes sense" is). But my point is that per-inode is not only the logically more straightforward place to do it, it's also the much more flexible place to do it. Because it *allows* for things like that. >>> >>> Ok. To summarize, S_IMA indicates that there is a rule and that the iint >>> was allocated. To differentiate between 'haven't looked/don't know' and >>> 'definitely not', we need another bit. For this, you're suggesting >>> using IS_PRIVATE()? Hopefully, I misunderstood. >>> >>> thanks, >>> >>> Mimi >>> >>> >>> -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/2] ima: policy search speedup
On Tue, Dec 11, 2012 at 10:08 PM, Eric Paris wrote: > S_PRIVATE is totally unacceptable as it has a meaning across all LSMs, > not just IMA. > > S_NOSEC means 'this is not setuid or setgid and we don't need to do > those checks on modify' > > You are going to need to use a S_NOIMA. > > Of Dmitry's 90,000 fewer policy lookups using the per sb flag, how > many of them are the same inode over and over again which would be > circumvented using S_NOIMA per inode flag? > IIRC those were only newly instantiated inodes. For new inodes, S_NOIMA would not be set and used at that point. IMA must do the policy search and then would set the S_NOIMA. For subsequent calls, S_NOIMA makes sense. If we would have the SB flag like MS_NOIMA, then we could replicate sb flag to inode S_NOIMA flag, in similar way as inode_has_no_xattr() does: static inline void inode_has_no_xattr(struct inode *inode) { if (!is_sxid(inode->i_mode) && (inode->i_sb->s_flags & MS_NOSEC)) inode->i_flags |= S_NOSEC; } Then later there is no need to dereference inode->i_sb... Can we reach conclusion about it? Will we have SB flag? if yes, then where, s_flags, or in some other field like s_feature_flags? if not, then we can stop this discussion... - Dmitry > > > On Tue, Dec 11, 2012 at 2:48 PM, Mimi Zohar wrote: >> On Tue, 2012-12-11 at 11:10 -0800, Linus Torvalds wrote: >> >>> Anyway, the whole "you can do it at file granularity" isn't the bulk >>> of my argument (the "we already have the field that makes sense" is). >>> But my point is that per-inode is not only the logically more >>> straightforward place to do it, it's also the much more flexible place >>> to do it. Because it *allows* for things like that. >> >> Ok. To summarize, S_IMA indicates that there is a rule and that the iint >> was allocated. To differentiate between 'haven't looked/don't know' and >> 'definitely not', we need another bit. For this, you're suggesting >> using IS_PRIVATE()? Hopefully, I misunderstood. >> >> thanks, >> >> Mimi >> >> >> -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/2] ima: policy search speedup
On Tue, Dec 11, 2012 at 02:07:22PM -0500, Mimi Zohar wrote: > On Tue, 2012-12-11 at 13:09 -0500, Eric Paris wrote: > > On Tue, Dec 11, 2012 at 12:55 PM, Linus Torvalds > > wrote: > > > > > And your "pseudo-filesystems" argument is pretty stupid too, since WE > > > ALREADY HAVE A FLAG FOR THAT! > > > > > > Guess where it is? Oh, it's in the place I already mentioned makes > > > more sense. Look for S_PRIVATE in inode->i_flags, and IS_PRIVATE() in > > > users. It's what the other security models already use to avoid > > > bothering calling down to the security layers. The fact that the > > > integrity layer bypasses the normal security layer in > > > ima_file_check(), for example, is no excuse to then make up totally > > > new flags. > > > > IS_PRIVATE() is not used by and darn well better not be used by, all > > psuedo filesystems like procfs which IMA may want to ignore. LSMs > > like to do control on them. I thought S_PRIVATE was really only used > > by the anon_inode and reiserfs's really crazy ass internal inodes. I > > could always be wrong. > > I was actually wondering about the MS_NOSEC flag. It's currently being > used by fuse, gfs2, ocfs2 and tmpfs. (Not sure about xfs.) Can someone > explain what it is being used for? For determining whether to clearing suid bits on writes to a file. It defaults to on for all filesystems that use mount_bdev(), which is just about every local filesystem (include XFS). Cheers, Dave. -- Dave Chinner da...@fromorbit.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/2] ima: policy search speedup
On Tue, Dec 11, 2012 at 12:15 PM, Eric Paris wrote: > > I know it isn't relevant to the final solution, but this is simply > wrong. IS_PRIVATE() means 'this inode is filesystem internal.' It is > not used by anything except rieser and the anon_inode. If it is used > by psuedo filesystems in general, like /proc, shmem mappings, and > pipes that is a huge bug and is absolutely wrong. Hmm.. The magic anonfs inode definitely sets it, as far as I can tell. But it turns out that I was wrong anyway, and you are largely right: pipes and sockets don't use the anonfs inode (they allocate their own inodes directly using 'new_inode_pseudo()'), and neither does /proc. So it's actually only signalfd and timerfd and some other special things like kvm internal file descriptors that use it. So never mind about S_PRIVATE, it has odd semantics. Linus -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/2] ima: policy search speedup
On Tue, Dec 11, 2012 at 3:05 PM, Linus Torvalds wrote: > The "IS_PRIVATE()" thing is more a "if you know a-priori that you > aren't interested in pseudo-filesystems, you can already check that > bit, because it will be set for things like /proc and shmem mappings > and pipes etc". I know it isn't relevant to the final solution, but this is simply wrong. IS_PRIVATE() means 'this inode is filesystem internal.' It is not used by anything except rieser and the anon_inode. If it is used by psuedo filesystems in general, like /proc, shmem mappings, and pipes that is a huge bug and is absolutely wrong. You are correct IS_PRIVATE is sufficient to know you don't need to do any IMA stuff with that inode, but it is used in damn few places and better remain that way -Eric -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/2] ima: policy search speedup
S_PRIVATE is totally unacceptable as it has a meaning across all LSMs, not just IMA. S_NOSEC means 'this is not setuid or setgid and we don't need to do those checks on modify' You are going to need to use a S_NOIMA. Of Dmitry's 90,000 fewer policy lookups using the per sb flag, how many of them are the same inode over and over again which would be circumvented using S_NOIMA per inode flag? On Tue, Dec 11, 2012 at 2:48 PM, Mimi Zohar wrote: > On Tue, 2012-12-11 at 11:10 -0800, Linus Torvalds wrote: > >> Anyway, the whole "you can do it at file granularity" isn't the bulk >> of my argument (the "we already have the field that makes sense" is). >> But my point is that per-inode is not only the logically more >> straightforward place to do it, it's also the much more flexible place >> to do it. Because it *allows* for things like that. > > Ok. To summarize, S_IMA indicates that there is a rule and that the iint > was allocated. To differentiate between 'haven't looked/don't know' and > 'definitely not', we need another bit. For this, you're suggesting > using IS_PRIVATE()? Hopefully, I misunderstood. > > thanks, > > Mimi > > > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/2] ima: policy search speedup
On Tue, Dec 11, 2012 at 11:48 AM, Mimi Zohar wrote: > > Ok. To summarize, S_IMA indicates that there is a rule and that the iint > was allocated. To differentiate between 'haven't looked/don't know' and > 'definitely not', we need another bit. For this, you're suggesting > using IS_PRIVATE()? Hopefully, I misunderstood. No, for that, I'm suggesting using a new bit in i_flags. The "IS_PRIVATE()" thing is more a "if you know a-priori that you aren't interested in pseudo-filesystems, you can already check that bit, because it will be set for things like /proc and shmem mappings and pipes etc". Dmitry seemed to imply that the biggest use for the new bit was for taking out whole pseudo-filesystems in one go. That would pretty much be what S_PRIVATE is. Linus -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/2] ima: policy search speedup
On Tue, 2012-12-11 at 11:10 -0800, Linus Torvalds wrote: > Anyway, the whole "you can do it at file granularity" isn't the bulk > of my argument (the "we already have the field that makes sense" is). > But my point is that per-inode is not only the logically more > straightforward place to do it, it's also the much more flexible place > to do it. Because it *allows* for things like that. Ok. To summarize, S_IMA indicates that there is a rule and that the iint was allocated. To differentiate between 'haven't looked/don't know' and 'definitely not', we need another bit. For this, you're suggesting using IS_PRIVATE()? Hopefully, I misunderstood. thanks, Mimi -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/2] ima: policy search speedup
On Tue, Dec 11, 2012 at 10:59 AM, Mimi Zohar wrote: > > Bottom line, > we can't say definitively whether or not a file needs to be measured for > any caller. .. but is that actually *always* the case? Is there some fundamental reason why integrity rules can never have "this file just doesn't matter"? For example, if you have cases where you know that a file has a particular policy (or no policy what-so-ever) that never cares, then for that file you can say "don't bother measuring this file" even though in the generic case that may not be true. Things like that might be the common case. Like "normal files owned by uid > 500 are user files, and we simply don't care, and fall back to just normal unix permissions". And yes, things like that may end up requiring that the flag be cleared on other inode events (ie changing file ownership of executable flags or whatever, which might change a file from "don't bother" to "hey, now it might be interesting again"). So maybe chmod/chown/chgrp would clear that flag.. Anyway, the whole "you can do it at file granularity" isn't the bulk of my argument (the "we already have the field that makes sense" is). But my point is that per-inode is not only the logically more straightforward place to do it, it's also the much more flexible place to do it. Because it *allows* for things like that. Linus -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/2] ima: policy search speedup
On Tue, 2012-12-11 at 13:09 -0500, Eric Paris wrote: > On Tue, Dec 11, 2012 at 12:55 PM, Linus Torvalds > wrote: > > > And your "pseudo-filesystems" argument is pretty stupid too, since WE > > ALREADY HAVE A FLAG FOR THAT! > > > > Guess where it is? Oh, it's in the place I already mentioned makes > > more sense. Look for S_PRIVATE in inode->i_flags, and IS_PRIVATE() in > > users. It's what the other security models already use to avoid > > bothering calling down to the security layers. The fact that the > > integrity layer bypasses the normal security layer in > > ima_file_check(), for example, is no excuse to then make up totally > > new flags. > > IS_PRIVATE() is not used by and darn well better not be used by, all > psuedo filesystems like procfs which IMA may want to ignore. LSMs > like to do control on them. I thought S_PRIVATE was really only used > by the anon_inode and reiserfs's really crazy ass internal inodes. I > could always be wrong. I was actually wondering about the MS_NOSEC flag. It's currently being used by fuse, gfs2, ocfs2 and tmpfs. (Not sure about xfs.) Can someone explain what it is being used for? thanks, Mimi -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/2] ima: policy search speedup
On Tue, 2012-12-11 at 13:35 -0500, Eric Paris wrote: > On Tue, Dec 11, 2012 at 1:18 PM, Mimi Zohar wrote: > > > The appraisal policy is based on the object metadata, such as the uid, > > so the result is static and can be cached. The measurement policy, on > > the other hand, is normally based on the subject (eg. who is > > reading/executing) the file. Knowledge of whether the file has been > > measured is cached in the iint, but unlike the appraisal policy, not > > whether it needs to be measured. Having the flag on a per inode basis, > > doesn't really help. > > Can you try again? Even I can't parse this. Not sure what to tell > you to try again, maybe give us a summary at a high level again and > then why this patch is specifically necessary? sigh. The IMA policy contains rules for the original IMA measurement, hash auditing, and now for IMA appraisal. These policies overlap with each other, but are not the same. Although the policy doesn't change, the rules can be dependent on the calling process. For example, the original default 'ima_tcb' policy is based, not on the file owner, but on the uid reading/executing the file. The 'ima_tcb' policy measures all files read/executed by root. So we cache whether the file has been measured, not if the file needs to be measured, because depending on the caller, that changes. Bottom line, we can't say definitively whether or not a file needs to be measured for any caller. Dmitry's patch addresses the issue of eliminating an entire filesystem from being appraised, measured, or audited. I hope this clarifies the issues a bit better. Mimi -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/2] ima: policy search speedup
On Tue, Dec 11, 2012 at 8:35 PM, Linus Torvalds wrote: > On Tue, Dec 11, 2012 at 10:12 AM, Kasatkin, Dmitry > wrote: >> >> Actually S_PRIVATE does not work work for normal filesystems which IMA >> might want to ignore. > > The reading comprehension here is abysmal. > > First you claim that you need the new flag for pseudo-filesystems, and > now that I point out that we have an *old* flag for pseudo-filesystems > you turn around 180 degrees and talk about other filesystems. > I have not claimed that. This is exactly what I have written: "There are different filesystems which are not checked by IMA/EVM, such as pseudo-filesystems" Pseudo-filesystems was an example of possible cases. Sorry if it was not clear enough. > And none of that matters for my argument AT ALL. > > My argument has not been that we cannot add a new flag. > > My argument has been that we already have the logical place for such a > flag, and that adding a totally new field seems so stupid. > > Seriously. The i_flags place is where we already do pretty much > *exactly* what you ask for. The fact that it is faster and more > flexible to boot should be a bonus. > > Now, there are real reasons to avoid "s_flags", notably the fact that > we're running out of bits there (unlike i_flags), and they are exposed > as generic fields and are generally meant for mount options etc. So I > understand why we might want to avoid that (although the whole > mount-option thing could also be seen as an advantage), but I really > don't see any argument against i_flags, considering that we already > use it for S_IMA and S_PRIVATE, both of which are related to exactly > what you seem to want to do. Not every inode on the filesystem might be checked by IMA. It depends on policy. And S_IMA flag is used exactly for this purpose. But when the entire FS is not checked, SB flag seems to be very appropriate. Eric has given nice explanation. > > The one downside of i_flags may be that any update should own the > inode semaphore. But within the context of a security model, that > should be fine (and normally you'd update it once per lifetime of the > inode). > This is exactly how IMA works at the moment. See my response to Eric about performance. > Linus -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/2] ima: policy search speedup
On Tue, Dec 11, 2012 at 10:12 AM, Kasatkin, Dmitry wrote: > > Actually S_PRIVATE does not work work for normal filesystems which IMA > might want to ignore. The reading comprehension here is abysmal. First you claim that you need the new flag for pseudo-filesystems, and now that I point out that we have an *old* flag for pseudo-filesystems you turn around 180 degrees and talk about other filesystems. And none of that matters for my argument AT ALL. My argument has not been that we cannot add a new flag. My argument has been that we already have the logical place for such a flag, and that adding a totally new field seems so stupid. Seriously. The i_flags place is where we already do pretty much *exactly* what you ask for. The fact that it is faster and more flexible to boot should be a bonus. Now, there are real reasons to avoid "s_flags", notably the fact that we're running out of bits there (unlike i_flags), and they are exposed as generic fields and are generally meant for mount options etc. So I understand why we might want to avoid that (although the whole mount-option thing could also be seen as an advantage), but I really don't see any argument against i_flags, considering that we already use it for S_IMA and S_PRIVATE, both of which are related to exactly what you seem to want to do. The one downside of i_flags may be that any update should own the inode semaphore. But within the context of a security model, that should be fine (and normally you'd update it once per lifetime of the inode). Linus -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/2] ima: policy search speedup
On Tue, Dec 11, 2012 at 1:18 PM, Mimi Zohar wrote: > The appraisal policy is based on the object metadata, such as the uid, > so the result is static and can be cached. The measurement policy, on > the other hand, is normally based on the subject (eg. who is > reading/executing) the file. Knowledge of whether the file has been > measured is cached in the iint, but unlike the appraisal policy, not > whether it needs to be measured. Having the flag on a per inode basis, > doesn't really help. Can you try again? Even I can't parse this. Not sure what to tell you to try again, maybe give us a summary at a high level again and then why this patch is specifically necessary? -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/2] ima: policy search speedup
On Tue, Dec 11, 2012 at 8:09 PM, Eric Paris wrote: > On Tue, Dec 11, 2012 at 12:55 PM, Linus Torvalds > wrote: > >> And your "pseudo-filesystems" argument is pretty stupid too, since WE >> ALREADY HAVE A FLAG FOR THAT! >> >> Guess where it is? Oh, it's in the place I already mentioned makes >> more sense. Look for S_PRIVATE in inode->i_flags, and IS_PRIVATE() in >> users. It's what the other security models already use to avoid >> bothering calling down to the security layers. The fact that the >> integrity layer bypasses the normal security layer in >> ima_file_check(), for example, is no excuse to then make up totally >> new flags. > > IS_PRIVATE() is not used by and darn well better not be used by, all > psuedo filesystems like procfs which IMA may want to ignore. LSMs > like to do control on them. I thought S_PRIVATE was really only used > by the anon_inode and reiserfs's really crazy ass internal inodes. I > could always be wrong. > > I don't know if I agree with dmitry but let me explain what's going on here. > > Lets say the user accesses an inode in procfs. Without this patch one > would search the ima policy and find a rule that says 'if this inode > is on procfs we don't care.' We can then cache that in the struct > inode like you say and move along. If another inode on procfs is > opened we will have the same policy search and the same per inode 'i > don't give a crap' marking. This absolutely works you are right. But > we search the IMA policy for every inode. > > With Dmitry's patch we can instead search the IMA policy one time and > mark the whole superblock as 'i don't give a crap' if IMA policy says > we don't care about that fstype. When the second procfs inode is > opened we instead look at the per superblock 'who gives a crap' and > never search the IMA policy. So we save all future policy searches. > > I'd say this patch would only be a good idea if there is a real > performance hit which is measurable in a real work situation. Not, > 'look how much faster it is to access proc inodes' microbenchmark, > since noone is actually going to do that, but some results of a useful > benchmark you care about. Maybe Dmitry gave those numbers and I > missed them? Otherwise, stick with per inode like Linus wants... I was measuring that during on the system without super block flag, policy search was happening 100 000 times, but with the flag just bellow 10 000. For desktop multi-core systems powered from the plug it might be unnoticeable. But for the handhald it might save the battery. - Dmitry -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/2] ima: policy search speedup
On Tue, Dec 11, 2012 at 08:10:07PM +0200, Kasatkin, Dmitry wrote: > I was suggesting to add a flag for sb->s_flags field. > Al said that he would not like this, because s_flags are mount specific. s/mount-specific/in an incestous relationship with mount(2) ABI/. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/2] ima: policy search speedup
On Tue, 2012-12-11 at 08:59 -0800, Linus Torvalds wrote: > On Tue, Dec 11, 2012 at 6:08 AM, Mimi Zohar wrote: > > On Tue, 2012-12-11 at 14:51 +0200, Kasatkin, Dmitry wrote: > >> >> > >> >> Two months ago I was asking about it on mailing lists. > >> >> Suggestion was not to use s_flags, but e.g. s_feature_flags. > > Quite frankly, this seems stupid. > > Without really knowing the problem space, the sane thing to do would > seem to be inode->i_flags. At which point it's > > (a) faster to test (no need to dereference inode->i_sb) > > (b) matches what the integrity layer does with S_IMA (well, there the > logic is reversed: S_IMA means that it has a integrity structure > associated with it) > > (c) allows you to mark individual inodes as "no checking". The appraisal policy is based on the object metadata, such as the uid, so the result is static and can be cached. The measurement policy, on the other hand, is normally based on the subject (eg. who is reading/executing) the file. Knowledge of whether the file has been measured is cached in the iint, but unlike the appraisal policy, not whether it needs to be measured. Having the flag on a per inode basis, doesn't really help. thanks, Mimi -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/2] ima: policy search speedup
On Tue, Dec 11, 2012 at 7:55 PM, Linus Torvalds wrote: > On Tue, Dec 11, 2012 at 9:40 AM, Kasatkin, Dmitry > wrote: >>> >>> Quite frankly, this seems stupid. >> >> What exactly seems stupid here? > > What I said. Go back and read it. I gave three reasons. Why do you ask? > > I'll give one more reason, but you probably won't read *this* email > either, will you? > >> There are different filesystems which are not checked by IMA/EVM, >> such as pseudo-filesystems. > > Did you read my email? > > There are probably *also* individual that aren't checked by IMA/EVM > even on filesystems that *do* check other files. > > No? > > And your "pseudo-filesystems" argument is pretty stupid too, since WE > ALREADY HAVE A FLAG FOR THAT! > > Guess where it is? Oh, it's in the place I already mentioned makes > more sense. Look for S_PRIVATE in inode->i_flags, and IS_PRIVATE() in > users. It's what the other security models already use to avoid > bothering calling down to the security layers. The fact that the > integrity layer bypasses the normal security layer in > ima_file_check(), for example, is no excuse to then make up totally > new flags. Actually S_PRIVATE does not work work for normal filesystems which IMA might want to ignore. > > So let me repeat: adding a new superblock flag seems STUPID. Why is it > in a completely different place than all the other flags that we > already have for these kinds of things? Why should we add a new field, > when we have existing fields that seem to do exactly this, do it > better, and are already used? > > And don't ask me why without reading this email. OK? > > Linus -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/2] ima: policy search speedup
On Tue, Dec 11, 2012 at 7:55 PM, Linus Torvalds wrote: > On Tue, Dec 11, 2012 at 9:40 AM, Kasatkin, Dmitry > wrote: >>> >>> Quite frankly, this seems stupid. >> >> What exactly seems stupid here? > > What I said. Go back and read it. I gave three reasons. Why do you ask? > > I'll give one more reason, but you probably won't read *this* email > either, will you? > No comments. Should I say more here? Better read further... >> There are different filesystems which are not checked by IMA/EVM, >> such as pseudo-filesystems. > > Did you read my email? > > There are probably *also* individual that aren't checked by IMA/EVM > even on filesystems that *do* check other files. > > No? > > And your "pseudo-filesystems" argument is pretty stupid too, since WE > ALREADY HAVE A FLAG FOR THAT! > > Guess where it is? Oh, it's in the place I already mentioned makes > more sense. Look for S_PRIVATE in inode->i_flags, and IS_PRIVATE() in > users. It's what the other security models already use to avoid > bothering calling down to the security layers. The fact that the > integrity layer bypasses the normal security layer in > ima_file_check(), for example, is no excuse to then make up totally > new flags. > > So let me repeat: adding a new superblock flag seems STUPID. Why is it > in a completely different place than all the other flags that we > already have for these kinds of things? Why should we add a new field, > when we have existing fields that seem to do exactly this, do it > better, and are already used? > I was suggesting to add a flag for sb->s_flags field. Al said that he would not like this, because s_flags are mount specific. One of suggestions in response was to use s_feature flags, because other FSs would benefit from that as well.. https://lkml.org/lkml/2012/9/19/460 > And don't ask me why without reading this email. OK? Again, no comments. > > Linus -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/2] ima: policy search speedup
On Tue, Dec 11, 2012 at 12:55 PM, Linus Torvalds wrote: > And your "pseudo-filesystems" argument is pretty stupid too, since WE > ALREADY HAVE A FLAG FOR THAT! > > Guess where it is? Oh, it's in the place I already mentioned makes > more sense. Look for S_PRIVATE in inode->i_flags, and IS_PRIVATE() in > users. It's what the other security models already use to avoid > bothering calling down to the security layers. The fact that the > integrity layer bypasses the normal security layer in > ima_file_check(), for example, is no excuse to then make up totally > new flags. IS_PRIVATE() is not used by and darn well better not be used by, all psuedo filesystems like procfs which IMA may want to ignore. LSMs like to do control on them. I thought S_PRIVATE was really only used by the anon_inode and reiserfs's really crazy ass internal inodes. I could always be wrong. I don't know if I agree with dmitry but let me explain what's going on here. Lets say the user accesses an inode in procfs. Without this patch one would search the ima policy and find a rule that says 'if this inode is on procfs we don't care.' We can then cache that in the struct inode like you say and move along. If another inode on procfs is opened we will have the same policy search and the same per inode 'i don't give a crap' marking. This absolutely works you are right. But we search the IMA policy for every inode. With Dmitry's patch we can instead search the IMA policy one time and mark the whole superblock as 'i don't give a crap' if IMA policy says we don't care about that fstype. When the second procfs inode is opened we instead look at the per superblock 'who gives a crap' and never search the IMA policy. So we save all future policy searches. I'd say this patch would only be a good idea if there is a real performance hit which is measurable in a real work situation. Not, 'look how much faster it is to access proc inodes' microbenchmark, since noone is actually going to do that, but some results of a useful benchmark you care about. Maybe Dmitry gave those numbers and I missed them? Otherwise, stick with per inode like Linus wants... -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/2] ima: policy search speedup
On Tue, Dec 11, 2012 at 9:40 AM, Kasatkin, Dmitry wrote: >> >> Quite frankly, this seems stupid. > > What exactly seems stupid here? What I said. Go back and read it. I gave three reasons. Why do you ask? I'll give one more reason, but you probably won't read *this* email either, will you? > There are different filesystems which are not checked by IMA/EVM, > such as pseudo-filesystems. Did you read my email? There are probably *also* individual that aren't checked by IMA/EVM even on filesystems that *do* check other files. No? And your "pseudo-filesystems" argument is pretty stupid too, since WE ALREADY HAVE A FLAG FOR THAT! Guess where it is? Oh, it's in the place I already mentioned makes more sense. Look for S_PRIVATE in inode->i_flags, and IS_PRIVATE() in users. It's what the other security models already use to avoid bothering calling down to the security layers. The fact that the integrity layer bypasses the normal security layer in ima_file_check(), for example, is no excuse to then make up totally new flags. So let me repeat: adding a new superblock flag seems STUPID. Why is it in a completely different place than all the other flags that we already have for these kinds of things? Why should we add a new field, when we have existing fields that seem to do exactly this, do it better, and are already used? And don't ask me why without reading this email. OK? Linus -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/2] ima: policy search speedup
On Tue, Dec 11, 2012 at 6:59 PM, Linus Torvalds wrote: > On Tue, Dec 11, 2012 at 6:08 AM, Mimi Zohar wrote: >> On Tue, 2012-12-11 at 14:51 +0200, Kasatkin, Dmitry wrote: >>> >> >>> >> Two months ago I was asking about it on mailing lists. >>> >> Suggestion was not to use s_flags, but e.g. s_feature_flags. > > Quite frankly, this seems stupid. What exactly seems stupid here? > > Without really knowing the problem space, the sane thing to do would > seem to be inode->i_flags. At which point it's > > (a) faster to test (no need to dereference inode->i_sb) > > (b) matches what the integrity layer does with S_IMA (well, there the > logic is reversed: S_IMA means that it has a integrity structure > associated with it) > > (c) allows you to mark individual inodes as "no checking". > There are inode specific objects which IMA uses for such perpose. > and quite frankly, (c) in particular seems to make sense to me, since > it would seem to be rather possible to do things like "I've checked > this inode, it had no policies associated with it, I never need to > check it again". Clear the flag when policies change or whatever. > > What's the advantage of making it per-filesystem? > There are different filesystems which are not checked by IMA/EVM, such as pseudo-filesystems. For this reason it is good to have a way to ignore such filesystems without to much work in IMA code. No reason to check policy again and again for every inode on the filesystem when the result will always be to ignore the filesystem. Per-filesystem flag soles this problem. - Dmitry > Linus > -- > To unsubscribe from this list: send the line "unsubscribe > linux-security-module" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/2] ima: policy search speedup
On Tue, Dec 11, 2012 at 6:08 AM, Mimi Zohar wrote: > On Tue, 2012-12-11 at 14:51 +0200, Kasatkin, Dmitry wrote: >> >> >> >> Two months ago I was asking about it on mailing lists. >> >> Suggestion was not to use s_flags, but e.g. s_feature_flags. Quite frankly, this seems stupid. Without really knowing the problem space, the sane thing to do would seem to be inode->i_flags. At which point it's (a) faster to test (no need to dereference inode->i_sb) (b) matches what the integrity layer does with S_IMA (well, there the logic is reversed: S_IMA means that it has a integrity structure associated with it) (c) allows you to mark individual inodes as "no checking". and quite frankly, (c) in particular seems to make sense to me, since it would seem to be rather possible to do things like "I've checked this inode, it had no policies associated with it, I never need to check it again". Clear the flag when policies change or whatever. What's the advantage of making it per-filesystem? Linus -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/2] ima: policy search speedup
On Tue, 2012-12-11 at 14:51 +0200, Kasatkin, Dmitry wrote: > >> Here is two patches for policy search speedup. > >> > >> First patch adds additional features flags to superblock. > >> Second - implementation for IMA. > >> > >> Two months ago I was asking about it on mailing lists. > >> Suggestion was not to use s_flags, but e.g. s_feature_flags. > >> > >> Any objections about such approach? First of all my appologies for not responding earlier. Based on the previous discussion https://lkml.org/lkml/2012/9/19/9, this looks like the right direction. Specific comments will be posted on the individual patches. thanks, Mimi -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/2] ima: policy search speedup
Hello Linus, Can you please comment on the feature flag in this patchset? Thanks, Dmitry On Tue, Nov 27, 2012 at 3:42 PM, Kasatkin, Dmitry wrote: > Hello, > > Any thoughts about this proposal? > > - Dmitry > > On Thu, Nov 22, 2012 at 11:54 PM, Dmitry Kasatkin > wrote: >> Hello, >> >> Here is two patches for policy search speedup. >> >> First patch adds additional features flags to superblock. >> Second - implementation for IMA. >> >> Two months ago I was asking about it on mailing lists. >> Suggestion was not to use s_flags, but e.g. s_feature_flags. >> >> Any objections about such approach? >> >> Thanks, >> Dmitry >> >> Dmitry Kasatkin (2): >> vfs: new super block feature flags attribute >> ima: skip policy search for never appraised or measured files >> >> include/linux/fs.h |4 >> security/integrity/ima/ima_api.c|8 ++-- >> security/integrity/ima/ima_policy.c | 20 +--- >> security/integrity/integrity.h |3 +++ >> 4 files changed, 26 insertions(+), 9 deletions(-) >> >> -- >> 1.7.10.4 >> -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/2] ima: policy search speedup
Hello, Any thoughts about this proposal? - Dmitry On Thu, Nov 22, 2012 at 11:54 PM, Dmitry Kasatkin wrote: > Hello, > > Here is two patches for policy search speedup. > > First patch adds additional features flags to superblock. > Second - implementation for IMA. > > Two months ago I was asking about it on mailing lists. > Suggestion was not to use s_flags, but e.g. s_feature_flags. > > Any objections about such approach? > > Thanks, > Dmitry > > Dmitry Kasatkin (2): > vfs: new super block feature flags attribute > ima: skip policy search for never appraised or measured files > > include/linux/fs.h |4 > security/integrity/ima/ima_api.c|8 ++-- > security/integrity/ima/ima_policy.c | 20 +--- > security/integrity/integrity.h |3 +++ > 4 files changed, 26 insertions(+), 9 deletions(-) > > -- > 1.7.10.4 > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/