| Message ID | CAJ-EccPGqp4PmRkFk505QhDKHWn-ajxS0__Nk9VS32jV_+3Y2A@mail.gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [GIT,PULL] SafeSetID LSM changes for 5.3 | expand |
On Mon, Jul 15, 2019 at 9:05 AM Micah Morton <mortonm@chromium.org> wrote: > > I'm maintaining the new SafeSetID LSM and was told to set up my own > tree for sending pull requests rather than sending my changes through > James Morris and the security subsystem tree. Yes. It would be good if you also added yourself to the MAINTAINERS file. Right now there's no entry for security/safesetid at all. > This is my first time doing one of these pull requests so hopefully I > didn't screw something up. So a couple of notes: - *please* don't rebase your work in the day before Was this in linux-next? was this tested at all? Hard to tell, since it was rebased recently, so for all I know it's all completely new - don't use a random kernel-of-the-day as the base for development This is related to the rebasing issue, but is true even if you don't rebase. There is no way that it was a good idea to pick my random - possibly completely broken - kernel from Sunday afternoon in the middle of a merge window as a base for development. If you start development, or if you have to rebase (for some *good* reason) you need to do so on a good stable base, not on the quick-sand that is "random kernel of the day during the busiest merge activity". - Please use the "git pull-request" format and then add any extra notes you feel are necessary Yes, your pull request is *almost* git pull-request, but you seem to have actively removed whitespace making it almost illegible. It's really hard to pick out the line that has the actual git repository address, because it's basically hidden inside one big blob of text. I've pulled this as-is since it's the first time, but I expect better next time. There are various resources on some cleanliness issues, and people fairly recently tried to combine it under Documentation/maintainer/rebasing-and-merging.rst which covers at least the basics on why not to rebase etc. And if you *do* end up rebasing, consider the end result "untested", so then it should have been done before the merge window even started, and the rebased branch should have been in linux-next. And not sent to me the very next day. Linus
On Tue, Jul 16, 2019 at 12:06 PM Linus Torvalds <torvalds@linux-foundation.org> wrote: > > - Please use the "git pull-request" format and then add any extra > notes you feel are necessary > > Yes, your pull request is *almost* git pull-request, but you seem > to have actively removed whitespace making it almost illegible. It's > really hard to pick out the line that has the actual git repository > address, because it's basically hidden inside one big blob of text. Extra note on this: you seem to have done "git pull-request" in a wide window, and then copied-and-pasted it into your MUA. So the diffstat lines are also very long, and then they line-wrap and it all looks nasty. Avoid this by either using a file for the output (that you then edit for your own added messages), or using a normal 80x25 terminal or something. I guess I should ask Junio to add a "--stat=80" to the upstream git request-pull script. Linus
The pull request you sent on Mon, 15 Jul 2019 09:04:48 -0700:
> https://github.com/micah-morton/linux.git tags/safesetid-5.3
has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/1ec4013bab89058dcc594dfe7b5a20f5d46bbc5f
Thank you!
On Tue, Jul 16, 2019 at 12:06 PM Linus Torvalds <torvalds@linux-foundation.org> wrote: > > On Mon, Jul 15, 2019 at 9:05 AM Micah Morton <mortonm@chromium.org> wrote: > > > > I'm maintaining the new SafeSetID LSM and was told to set up my own > > tree for sending pull requests rather than sending my changes through > > James Morris and the security subsystem tree. > > Yes. It would be good if you also added yourself to the MAINTAINERS > file. Right now there's no entry for security/safesetid at all. Yes, I have a patch for this but was told it would be better to send the patch through my tree rather than the security tree. I can send a pull request for that. > > > This is my first time doing one of these pull requests so hopefully I > > didn't screw something up. > > So a couple of notes: > > - *please* don't rebase your work in the day before Got it. > > Was this in linux-next? was this tested at all? Hard to tell, since > it was rebased recently, so for all I know it's all completely new This was not in linux-next, but was tested by Jann on a Chrome OS device. There's also the selftest for this code. But I can send non-trivial stuff to linux-next first next time. > > - don't use a random kernel-of-the-day as the base for development Got it. > > This is related to the rebasing issue, but is true even if you > don't rebase. There is no way that it was a good idea to pick my > random - possibly completely broken - kernel from Sunday afternoon in > the middle of a merge window as a base for development. > > If you start development, or if you have to rebase (for some *good* > reason) you need to do so on a good stable base, not on the quick-sand > that is "random kernel of the day during the busiest merge activity". Makes sense. The development was not actually done on that kernel, I just grabbed that random kernel for committing the changes on top of (these changes were developed a little while ago, but they're all self contained to the SafeSetID LSM), but I'll pick a stable one next time. > > - Please use the "git pull-request" format and then add any extra > notes you feel are necessary > > Yes, your pull request is *almost* git pull-request, but you seem > to have actively removed whitespace making it almost illegible. It's > really hard to pick out the line that has the actual git repository > address, because it's basically hidden inside one big blob of text. > > I've pulled this as-is since it's the first time, but I expect better next time. > > There are various resources on some cleanliness issues, and people > fairly recently tried to combine it under > > Documentation/maintainer/rebasing-and-merging.rst > > which covers at least the basics on why not to rebase etc. Thanks for the pointer. I had not seen that yet. > > And if you *do* end up rebasing, consider the end result "untested", > so then it should have been done before the merge window even started, > and the rebased branch should have been in linux-next. And not sent to > me the very next day. Yep, makes sense. > > Linus Thanks!
Hi Linus, I'm maintaining the new SafeSetID LSM and was told to set up my own tree for sending pull requests rather than sending my changes through James Morris and the security subsystem tree. This is my first time doing one of these pull requests so hopefully I didn't screw something up. Thanks, Micah --- The following changes since commit fec88ab0af9706b2201e5daf377c5031c62d11f7: Merge tag 'for-linus-hmm' of git://git.kernel.org/pub/scm/linux/kernel/git/rdma/rdma (2019-07-14 19:42:11 -0700) are available in the Git repository at: https://github.com/micah-morton/linux.git tags/safesetid-5.3 for you to fetch changes up to e10337daefecb47209fd2af5f4fab0d1a370737f: LSM: SafeSetID: fix use of literal -1 in capable hook (2019-07-15 08:08:03 -0700) ---------------------------------------------------------------- SafeSetID patches for 5.3 These changes from Jann Horn fix a couple issues in the recently added SafeSetID LSM: (1) There was a simple logic bug in one of the hooks for the LSM where the code was incorrectly returning early in some cases before all security checks had been passed. (2) There was a more high level issue with how this LSM gets configured that could allow for a program to bypass the security restrictions by switching to an allowed UID and then again to any other UID on the system if the target UID of the first transition is unconstrained on the system. Luckily this is an easy fix that we now enforce at the time the LSM gets configured. There are also some changes from Jann that make policy updates for this LSM atomic. Kees Cook, Jann and myself have reviewed these changes and they look good from our point of view. Signed-off-by: Micah Morton <mortonm@chromium.org> ---------------------------------------------------------------- Jann Horn (10): LSM: SafeSetID: fix pr_warn() to include newline LSM: SafeSetID: fix check for setresuid(new1, new2, new3) LSM: SafeSetID: refactor policy hash table LSM: SafeSetID: refactor safesetid_security_capable() LSM: SafeSetID: refactor policy parsing LSM: SafeSetID: fix userns handling in securityfs LSM: SafeSetID: rewrite userspace API to atomic updates LSM: SafeSetID: add read handler LSM: SafeSetID: verify transitive constrainedness LSM: SafeSetID: fix use of literal -1 in capable hook security/safesetid/lsm.c | 276 +++++++++++++----------------------------- security/safesetid/lsm.h | 34 ++++-- security/safesetid/securityfs.c | 307 +++++++++++++++++++++++++++++------------------ tools/testing/selftests/safesetid/safesetid-test.c | 18 ++- 4 files changed, 306 insertions(+), 329 deletions(-)