Re: [systemd-devel] [PATCH] ima-setup: write policy one line at a time

2015-06-11 Thread Zbigniew Jędrzejewski-Szmek 
On Thu, Jun 11, 2015 at 11:28:06AM +0200, Lennart Poettering wrote:
> On Thu, 11.06.15 00:34, Zbigniew Jędrzejewski-Szmek (zbys...@in.waw.pl) wrote:
> 
> > On Thu, Jun 11, 2015 at 01:16:47AM +0200, Lennart Poettering wrote:
> > > On Wed, 10.06.15 15:38, Zbigniew Jędrzejewski-Szmek (zbys...@in.waw.pl) 
> > > wrote:
> > > 
> > > > ima_write_policy() expects data to be written as one or more
> > > > rules, no more than PAGE_SIZE at a time. Easiest way to ensure
> > > > that we are not splitting rules is to read and write on line at
> > > > a time.
> > > > 
> > > > https://bugzilla.redhat.com/show_bug.cgi?id=1226948
> > > > ---
> > > >  src/core/ima-setup.c | 39 +--
> > > >  1 file changed, 17 insertions(+), 22 deletions(-)
> > > > 
> > > > diff --git a/src/core/ima-setup.c b/src/core/ima-setup.c
> > > > index 4d8b638115..5b3d16cd31 100644
> > > > --- a/src/core/ima-setup.c
> > > > +++ b/src/core/ima-setup.c
> > > > @@ -23,9 +23,6 @@
> > > >  
> > > >  #include 
> > > >  #include 
> > > > -#include 
> > > > -#include 
> > > > -#include 
> > > >  
> > > >  #include "ima-setup.h"
> > > >  #include "util.h"
> > > > @@ -36,20 +33,19 @@
> > > >  #define IMA_POLICY_PATH "/etc/ima/ima-policy"
> > > >  
> > > >  int ima_setup(void) {
> > > > -int r = 0;
> > > > -
> > > >  #ifdef HAVE_IMA
> > > > -_cleanup_close_ int policyfd = -1, imafd = -1;
> > > > -struct stat st;
> > > > -char *policy;
> > > > +_cleanup_fclose_ FILE *input = NULL;
> > > > +_cleanup_close_ int imafd = -1;
> > > > +char line[LINE_MAX];
> > > 
> > > Hmm, I wonder if this might bite us. LINE_MAX is a good choice as max
> > > line length for formats we define in systemd, but the question of
> > > course is what the the max line length is for IMA...
> >
> > It's PAGE_SIZE ;) Making this dynamic doesn't make much sense to me,
> > but we could make it 4096, as this is the lowest (and common) size.
> 
> I don't think this is actually really that bad:
> 
> _cleanup_free_ void *line = NULL;
> line = malloc(page_size());
> 
> Or, we could even just do alloca(page_size())...
Either would break FOR_EACH_LINE, but line[page_size()] should work.

https://github.com/systemd/systemd/pull/167

What I don't like about having a non-fixed value for the line size is
that the syntactic validity of configuration depends on the kernel you
are running. In practice not an issue, unless you like really long
lines.

@Mimi: could you check that the patch works for you?

Zbyszek
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] [PATCH] ima-setup: write policy one line at a time

2015-06-11 Thread Lennart Poettering 
On Thu, 11.06.15 00:34, Zbigniew Jędrzejewski-Szmek (zbys...@in.waw.pl) wrote:

> On Thu, Jun 11, 2015 at 01:16:47AM +0200, Lennart Poettering wrote:
> > On Wed, 10.06.15 15:38, Zbigniew Jędrzejewski-Szmek (zbys...@in.waw.pl) 
> > wrote:
> > 
> > > ima_write_policy() expects data to be written as one or more
> > > rules, no more than PAGE_SIZE at a time. Easiest way to ensure
> > > that we are not splitting rules is to read and write on line at
> > > a time.
> > > 
> > > https://bugzilla.redhat.com/show_bug.cgi?id=1226948
> > > ---
> > >  src/core/ima-setup.c | 39 +--
> > >  1 file changed, 17 insertions(+), 22 deletions(-)
> > > 
> > > diff --git a/src/core/ima-setup.c b/src/core/ima-setup.c
> > > index 4d8b638115..5b3d16cd31 100644
> > > --- a/src/core/ima-setup.c
> > > +++ b/src/core/ima-setup.c
> > > @@ -23,9 +23,6 @@
> > >  
> > >  #include 
> > >  #include 
> > > -#include 
> > > -#include 
> > > -#include 
> > >  
> > >  #include "ima-setup.h"
> > >  #include "util.h"
> > > @@ -36,20 +33,19 @@
> > >  #define IMA_POLICY_PATH "/etc/ima/ima-policy"
> > >  
> > >  int ima_setup(void) {
> > > -int r = 0;
> > > -
> > >  #ifdef HAVE_IMA
> > > -_cleanup_close_ int policyfd = -1, imafd = -1;
> > > -struct stat st;
> > > -char *policy;
> > > +_cleanup_fclose_ FILE *input = NULL;
> > > +_cleanup_close_ int imafd = -1;
> > > +char line[LINE_MAX];
> > 
> > Hmm, I wonder if this might bite us. LINE_MAX is a good choice as max
> > line length for formats we define in systemd, but the question of
> > course is what the the max line length is for IMA...
>
> It's PAGE_SIZE ;) Making this dynamic doesn't make much sense to me,
> but we could make it 4096, as this is the lowest (and common) size.

I don't think this is actually really that bad:

_cleanup_free_ void *line = NULL;
line = malloc(page_size());

Or, we could even just do alloca(page_size())...

Lennart

-- 
Lennart Poettering, Red Hat
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] [PATCH] ima-setup: write policy one line at a time

2015-06-10 Thread Zbigniew Jędrzejewski-Szmek 
On Thu, Jun 11, 2015 at 01:16:47AM +0200, Lennart Poettering wrote:
> On Wed, 10.06.15 15:38, Zbigniew Jędrzejewski-Szmek (zbys...@in.waw.pl) wrote:
> 
> > ima_write_policy() expects data to be written as one or more
> > rules, no more than PAGE_SIZE at a time. Easiest way to ensure
> > that we are not splitting rules is to read and write on line at
> > a time.
> > 
> > https://bugzilla.redhat.com/show_bug.cgi?id=1226948
> > ---
> >  src/core/ima-setup.c | 39 +--
> >  1 file changed, 17 insertions(+), 22 deletions(-)
> > 
> > diff --git a/src/core/ima-setup.c b/src/core/ima-setup.c
> > index 4d8b638115..5b3d16cd31 100644
> > --- a/src/core/ima-setup.c
> > +++ b/src/core/ima-setup.c
> > @@ -23,9 +23,6 @@
> >  
> >  #include 
> >  #include 
> > -#include 
> > -#include 
> > -#include 
> >  
> >  #include "ima-setup.h"
> >  #include "util.h"
> > @@ -36,20 +33,19 @@
> >  #define IMA_POLICY_PATH "/etc/ima/ima-policy"
> >  
> >  int ima_setup(void) {
> > -int r = 0;
> > -
> >  #ifdef HAVE_IMA
> > -_cleanup_close_ int policyfd = -1, imafd = -1;
> > -struct stat st;
> > -char *policy;
> > +_cleanup_fclose_ FILE *input = NULL;
> > +_cleanup_close_ int imafd = -1;
> > +char line[LINE_MAX];
> 
> Hmm, I wonder if this might bite us. LINE_MAX is a good choice as max
> line length for formats we define in systemd, but the question of
> course is what the the max line length is for IMA...
It's PAGE_SIZE ;) Making this dynamic doesn't make much sense to me,
but we could make it 4096, as this is the lowest (and common) size.

Zbyszek
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] [PATCH] ima-setup: write policy one line at a time

2015-06-10 Thread Lennart Poettering 
On Wed, 10.06.15 15:38, Zbigniew Jędrzejewski-Szmek (zbys...@in.waw.pl) wrote:

> ima_write_policy() expects data to be written as one or more
> rules, no more than PAGE_SIZE at a time. Easiest way to ensure
> that we are not splitting rules is to read and write on line at
> a time.
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1226948
> ---
>  src/core/ima-setup.c | 39 +--
>  1 file changed, 17 insertions(+), 22 deletions(-)
> 
> diff --git a/src/core/ima-setup.c b/src/core/ima-setup.c
> index 4d8b638115..5b3d16cd31 100644
> --- a/src/core/ima-setup.c
> +++ b/src/core/ima-setup.c
> @@ -23,9 +23,6 @@
>  
>  #include 
>  #include 
> -#include 
> -#include 
> -#include 
>  
>  #include "ima-setup.h"
>  #include "util.h"
> @@ -36,20 +33,19 @@
>  #define IMA_POLICY_PATH "/etc/ima/ima-policy"
>  
>  int ima_setup(void) {
> -int r = 0;
> -
>  #ifdef HAVE_IMA
> -_cleanup_close_ int policyfd = -1, imafd = -1;
> -struct stat st;
> -char *policy;
> +_cleanup_fclose_ FILE *input = NULL;
> +_cleanup_close_ int imafd = -1;
> +char line[LINE_MAX];

Hmm, I wonder if this might bite us. LINE_MAX is a good choice as max
line length for formats we define in systemd, but the question of
course is what the the max line length is for IMA...

Looks good otherwise.

Lennart

-- 
Lennart Poettering, Red Hat
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] [PATCH] ima-setup: write policy one line at a time

2015-06-10 Thread systemd github import bot 
Patchset imported to github.
To create a pull request, one of the main developers has to initiate one via:


--
Generated by https://github.com/haraldh/mail2git
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

[systemd-devel] [PATCH] ima-setup: write policy one line at a time

2015-06-10 Thread Zbigniew Jędrzejewski-Szmek 
ima_write_policy() expects data to be written as one or more
rules, no more than PAGE_SIZE at a time. Easiest way to ensure
that we are not splitting rules is to read and write on line at
a time.

https://bugzilla.redhat.com/show_bug.cgi?id=1226948
---
 src/core/ima-setup.c | 39 +--
 1 file changed, 17 insertions(+), 22 deletions(-)

diff --git a/src/core/ima-setup.c b/src/core/ima-setup.c
index 4d8b638115..5b3d16cd31 100644
--- a/src/core/ima-setup.c
+++ b/src/core/ima-setup.c
@@ -23,9 +23,6 @@
 
 #include 
 #include 
-#include 
-#include 
-#include 
 
 #include "ima-setup.h"
 #include "util.h"
@@ -36,20 +33,19 @@
 #define IMA_POLICY_PATH "/etc/ima/ima-policy"
 
 int ima_setup(void) {
-int r = 0;
-
 #ifdef HAVE_IMA
-_cleanup_close_ int policyfd = -1, imafd = -1;
-struct stat st;
-char *policy;
+_cleanup_fclose_ FILE *input = NULL;
+_cleanup_close_ int imafd = -1;
+char line[LINE_MAX];
+unsigned lineno = 0;
 
 if (access(IMA_SECFS_DIR, F_OK) < 0) {
 log_debug("IMA support is disabled in the kernel, ignoring.");
 return 0;
 }
 
-policyfd = open(IMA_POLICY_PATH, O_RDONLY|O_CLOEXEC);
-if (policyfd < 0) {
+input = fopen(IMA_POLICY_PATH, "re");
+if (!input) {
 log_full_errno(errno == ENOENT ? LOG_DEBUG : LOG_WARNING, 
errno,
"Failed to open the IMA custom policy file 
"IMA_POLICY_PATH", ignoring: %m");
 return 0;
@@ -66,20 +62,19 @@ int ima_setup(void) {
 return 0;
 }
 
-if (fstat(policyfd, &st) < 0)
-return log_error_errno(errno, "Failed to fstat 
"IMA_POLICY_PATH": %m");
+FOREACH_LINE(line, input,
+ return log_error_errno(errno, "Failed to read the IMA 
custom policy file "IMA_POLICY_PATH": %m")) {
+size_t len;
 
-policy = mmap(NULL, st.st_size, PROT_READ, MAP_PRIVATE, policyfd, 0);
-if (policy == MAP_FAILED)
-return log_error_errno(errno, "Failed to mmap 
"IMA_POLICY_PATH": %m");
+len = strlen(line);
+lineno++;
 
-r = loop_write(imafd, policy, (size_t) st.st_size, false);
-if (r < 0)
-log_error_errno(r, "Failed to load the IMA custom policy file 
"IMA_POLICY_PATH": %m");
-else
-log_info("Successfully loaded the IMA custom policy 
"IMA_POLICY_PATH".");
+if (len > 0 && write(imafd, line, len) < 0)
+return log_error_errno(errno, "Failed to load the IMA 
custom policy file "IMA_POLICY_PATH"%u: %m",
+   lineno);
+}
 
-munmap(policy, st.st_size);
+log_info("Successfully loaded the IMA custom policy 
"IMA_POLICY_PATH".");
 #endif /* HAVE_IMA */
-return r;
+return 0;
 }
-- 
2.1.0

___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel