[apparmor] [PATCH 10/11] Fix caching when used with a newer kernel with the feature directory
Kees Cook
kees at ubuntu.com
Thu Mar 8 22:11:57 UTC 2012
On Thu, Mar 08, 2012 at 01:57:44PM -0800, John Johansen wrote:
> On 03/08/2012 01:46 PM, Kees Cook wrote:
> > On Thu, Mar 08, 2012 at 01:40:54PM -0800, John Johansen wrote:
> >> On 03/08/2012 11:17 AM, Steve Beattie wrote:
> >>> On Thu, Mar 08, 2012 at 11:15:12AM -0800, Steve Beattie wrote:
> >>>> On Wed, Mar 07, 2012 at 06:17:29AM -0800, John Johansen wrote:
> >>>>> On newer kernels the features directory causes the creation of a
> >>>>> cache/.feature file that contains newline characters. This causes the
> >>>>> feature comparison to fail, because get_flags_string() uses fgets
> >>>>> which stop reading in the feature file after the first newline.
> >>>>>
> >>>>> This caches the features comparision to compare a single line of the
> >>>>> file against the full kernel feature directory resulting in caching
> >>>>> failure.
> >>>>>
> >>>>> Worse this also means the cache won't get updated as the parser doesn't
> >>>>> change what set gets caches after the .feature file gets created.
> >>>>>
> >>>>> Signed-off-by: John Johansen <john.johansen at canonical.com>
> >>>>
> >>>> Acked-By: Steve Beattie <sbeattie at ubuntu.com>
> >>>
> >>> Actually, fread() doesn't null terminate what it reads like fgets()
> >>> does. I think you'll need to address that.
> >>>
> >> How about with this additional patch on top
> >>
> >> diff --git a/parser/parser_main.c b/parser/parser_main.c
> >> index 67ab231..3f4789b 100644
> >> --- a/parser/parser_main.c
> >> +++ b/parser/parser_main.c
> >> @@ -844,6 +844,7 @@ out:
> >> static void get_flags_string(char **flags, char *flags_file) {
> >> char *pos;
> >> FILE *f = NULL;
> >> + size_t size;
> >>
> >> /* abort if missing or already set */
> >> if (!flags || *flags)
> >> @@ -857,8 +858,10 @@ static void get_flags_string(char **flags, char *flags_file) {
> >> if (!*flags)
> >> goto fail;
> >>
> >> - if (!fread(*flags, 1, FLAGS_STRING_SIZE, f))
> >> + size = fread(*flags, 1, FLAGS_STRING_SIZE - 1, f);
> >> + if (ferror(f))
> >
> > How about:
> >
> > if (size < 1 || ferror(f))
> > ...
> >
> >
> >> goto fail;
> >> + *flags[size] = 0;
> >
> > Then this can't freak out.
> >
> hrmmm well it shouldn't freak out if size == 0 and that is the only value less than 1 it
> could be, but from read fread again, yeah we should be checking for 0
Ah, yeah, good point. size < 0 should be fine.
--
Kees Cook
More information about the AppArmor
mailing list