| Message ID | 1291414464-6518-3-git-send-email-john.johansen@canonical.com |
|---|---|
| State | Accepted |
| Delegated to: | Brad Figg |
| Headers | show |
On 12/03/2010 03:14 PM, John Johansen wrote: > BugLink: http://bugs.launchpad.net/bugs/581525 > > SRU Justification: > See bug link for full justification > > NOTE: > This patch is OPTIONAL, and is only needed if the user space tools have > not been updated. The userspace tools update has been pulled in, so > unless there is the need to support users updating the kernel without > updating the userspace tools, I am NOT recommending this patch for > SRU, but including it for completeness. > > If the the patch to fix the bounds check on the next/check table has > been applied, and the userspace has not been updated. The it is likely > that part or all of policy will fail to load due to the bounds check > rejecting policy. > > The Justification for this patch is that it supports using a non-updated > (broken) userspace with a kernel that has the kernel fix for bug#581525 > applied. > > The apparmor_parser when compiling policy could generate invalid dfas > that did not have sufficient padding to avoid invalid references, when > used by the kernel. The kernels check to verify the next/check table > size was broken meaning invalid dfas were being created by userspace > and not caught. > > To remain compatible with old tools that are not fixed, pad the loaded > dfas next/check table. The dfa's themselves are valid except for the > high padding for potentially invalid transitions (high bounds error), > which have a maximimum is 256 entries. So just allocate an extra null filled > 256 entries for the next/check tables. This will guarentee all bounds > are good and invalid transitions go to the null (0) state. > > Signed-off-by: John Johansen<john.johansen@canonical.com> > --- > security/apparmor/include/apparmor.h | 6 ++++++ > security/apparmor/match.c | 9 ++++++++- > 2 files changed, 14 insertions(+), 1 deletions(-) > > diff --git a/security/apparmor/include/apparmor.h b/security/apparmor/include/apparmor.h > index 3f8d866..b4ddc95 100644 > --- a/security/apparmor/include/apparmor.h > +++ b/security/apparmor/include/apparmor.h > @@ -46,6 +46,12 @@ extern unsigned int aa_g_path_max; > printk(KERN_ERR "AppArmor: " fmt, ##args); \ > } while (0) > > +#define AA_WARN(fmt, args...) \ > + do { \ > + if (printk_ratelimit()) \ > + printk(KERN_WARN "AppArmor: " fmt, ##args); \ > + } while (0) > + > /* Flag indicating whether initialization completed */ > extern int apparmor_initialized; > void apparmor_disable(void); > diff --git a/security/apparmor/match.c b/security/apparmor/match.c > index b456495..f4486c1 100644 > --- a/security/apparmor/match.c > +++ b/security/apparmor/match.c > @@ -91,6 +91,13 @@ static struct table_header *unpack_table(char *blob, size_t bsize) > if (bsize< tsize) > goto out; > > + /* Pad table allocation for next/check by 256 entries to remain > + * backwards compatible with old (buggy) tools and remain safe without > + * run time checks > + */ > + if (th.td_id == YYTD_ID_NXT || th.td_id == YYTD_ID_CHK) > + tsize += 256 * th.td_flags; > + > /* freed by free_table */ > table = kmalloc(tsize, GFP_KERNEL | __GFP_NOWARN); > if (!table) { > @@ -178,7 +185,7 @@ static int verify_dfa(struct aa_dfa *dfa, int flags) > goto out; > /* TODO: do check that DEF state recursion terminates */ > if (BASE_TABLE(dfa)[i] + 255>= trans_count) { > - printk("AppArmor DFA next/check upper bounds error\n"); > + AA_WARN("DFA next/check upper bounds fixed, upgrade user space tools\n"); > goto out; > } > } I'm having a hard time parsing the commit log message. Are you trying to say that the Lucid kernel won't work if the current Maverick apparmor user space is backported to Lucid? Confused... rtg
On 12/06/2010 12:16 PM, Tim Gardner wrote: > On 12/03/2010 03:14 PM, John Johansen wrote: >> BugLink: http://bugs.launchpad.net/bugs/581525 >> >> SRU Justification: >> See bug link for full justification >> >> NOTE: >> This patch is OPTIONAL, and is only needed if the user space tools have >> not been updated. The userspace tools update has been pulled in, so >> unless there is the need to support users updating the kernel without >> updating the userspace tools, I am NOT recommending this patch for >> SRU, but including it for completeness. >> >> If the the patch to fix the bounds check on the next/check table has >> been applied, and the userspace has not been updated. The it is likely >> that part or all of policy will fail to load due to the bounds check >> rejecting policy. >> >> The Justification for this patch is that it supports using a non-updated >> (broken) userspace with a kernel that has the kernel fix for bug#581525 >> applied. >> >> The apparmor_parser when compiling policy could generate invalid dfas >> that did not have sufficient padding to avoid invalid references, when >> used by the kernel. The kernels check to verify the next/check table >> size was broken meaning invalid dfas were being created by userspace >> and not caught. >> >> To remain compatible with old tools that are not fixed, pad the loaded >> dfas next/check table. The dfa's themselves are valid except for the >> high padding for potentially invalid transitions (high bounds error), >> which have a maximimum is 256 entries. So just allocate an extra null filled >> 256 entries for the next/check tables. This will guarentee all bounds >> are good and invalid transitions go to the null (0) state. >> >> Signed-off-by: John Johansen<john.johansen@canonical.com> >> --- >> security/apparmor/include/apparmor.h | 6 ++++++ >> security/apparmor/match.c | 9 ++++++++- >> 2 files changed, 14 insertions(+), 1 deletions(-) >> >> diff --git a/security/apparmor/include/apparmor.h b/security/apparmor/include/apparmor.h >> index 3f8d866..b4ddc95 100644 >> --- a/security/apparmor/include/apparmor.h >> +++ b/security/apparmor/include/apparmor.h >> @@ -46,6 +46,12 @@ extern unsigned int aa_g_path_max; >> printk(KERN_ERR "AppArmor: " fmt, ##args); \ >> } while (0) >> >> +#define AA_WARN(fmt, args...) \ >> + do { \ >> + if (printk_ratelimit()) \ >> + printk(KERN_WARN "AppArmor: " fmt, ##args); \ >> + } while (0) >> + >> /* Flag indicating whether initialization completed */ >> extern int apparmor_initialized; >> void apparmor_disable(void); >> diff --git a/security/apparmor/match.c b/security/apparmor/match.c >> index b456495..f4486c1 100644 >> --- a/security/apparmor/match.c >> +++ b/security/apparmor/match.c >> @@ -91,6 +91,13 @@ static struct table_header *unpack_table(char *blob, size_t bsize) >> if (bsize< tsize) >> goto out; >> >> + /* Pad table allocation for next/check by 256 entries to remain >> + * backwards compatible with old (buggy) tools and remain safe without >> + * run time checks >> + */ >> + if (th.td_id == YYTD_ID_NXT || th.td_id == YYTD_ID_CHK) >> + tsize += 256 * th.td_flags; >> + >> /* freed by free_table */ >> table = kmalloc(tsize, GFP_KERNEL | __GFP_NOWARN); >> if (!table) { >> @@ -178,7 +185,7 @@ static int verify_dfa(struct aa_dfa *dfa, int flags) >> goto out; >> /* TODO: do check that DEF state recursion terminates */ >> if (BASE_TABLE(dfa)[i] + 255>= trans_count) { >> - printk("AppArmor DFA next/check upper bounds error\n"); >> + AA_WARN("DFA next/check upper bounds fixed, upgrade user space tools\n"); >> goto out; >> } >> } > > I'm having a hard time parsing the commit log message. Are you trying to say that the Lucid kernel won't work if the current Maverick apparmor user space is backported to Lucid? Confused... > no. I am saying there is a bug in the Lucid user space that will cause policy loading to some times break if the previous patch 1 is applied and either the Lucid userspace is not updated, or this patch #2 is not applied The fix for Lucid userspace is in -proposed, and as long as it goes to updates before a kernel with patch 1 hits -updates this patch is NOT needed unless, we care about the odd situation where a user installs an updated kernel but doesn't update their userspace. If the maverick userspace is used with the Lucid kernel with or without these patches everything is fine.
On 12/06/2010 01:49 PM, John Johansen wrote: > On 12/06/2010 12:16 PM, Tim Gardner wrote: >> On 12/03/2010 03:14 PM, John Johansen wrote: >>> BugLink: http://bugs.launchpad.net/bugs/581525 >>> >>> SRU Justification: See bug link for full justification >>> >>> NOTE: This patch is OPTIONAL, and is only needed if the user >>> space tools have not been updated. The userspace tools update >>> has been pulled in, so unless there is the need to support users >>> updating the kernel without updating the userspace tools, I am >>> NOT recommending this patch for SRU, but including it for >>> completeness. >>> >>> If the the patch to fix the bounds check on the next/check table >>> has been applied, and the userspace has not been updated. The it >>> is likely that part or all of policy will fail to load due to the >>> bounds check rejecting policy. >>> >>> The Justification for this patch is that it supports using a >>> non-updated (broken) userspace with a kernel that has the kernel >>> fix for bug#581525 applied. >>> >>> The apparmor_parser when compiling policy could generate invalid >>> dfas that did not have sufficient padding to avoid invalid >>> references, when used by the kernel. The kernels check to verify >>> the next/check table size was broken meaning invalid dfas were >>> being created by userspace and not caught. >>> >>> To remain compatible with old tools that are not fixed, pad the >>> loaded dfas next/check table. The dfa's themselves are valid >>> except for the high padding for potentially invalid transitions >>> (high bounds error), which have a maximimum is 256 entries. So >>> just allocate an extra null filled 256 entries for the next/check >>> tables. This will guarentee all bounds are good and invalid >>> transitions go to the null (0) state. >>> >>> Signed-off-by: John Johansen<john.johansen@canonical.com> --- >>> security/apparmor/include/apparmor.h | 6 ++++++ >>> security/apparmor/match.c | 9 ++++++++- 2 files >>> changed, 14 insertions(+), 1 deletions(-) >>> >>> diff --git a/security/apparmor/include/apparmor.h >>> b/security/apparmor/include/apparmor.h index 3f8d866..b4ddc95 >>> 100644 --- a/security/apparmor/include/apparmor.h +++ >>> b/security/apparmor/include/apparmor.h @@ -46,6 +46,12 @@ extern >>> unsigned int aa_g_path_max; printk(KERN_ERR "AppArmor: " fmt, >>> ##args); \ } while (0) >>> >>> +#define AA_WARN(fmt, args...) \ + do { >>> \ + if (printk_ratelimit()) \ + >>> printk(KERN_WARN "AppArmor: " fmt, ##args); \ + } while >>> (0) + /* Flag indicating whether initialization completed */ >>> extern int apparmor_initialized; void apparmor_disable(void); >>> diff --git a/security/apparmor/match.c >>> b/security/apparmor/match.c index b456495..f4486c1 100644 --- >>> a/security/apparmor/match.c +++ b/security/apparmor/match.c @@ >>> -91,6 +91,13 @@ static struct table_header *unpack_table(char >>> *blob, size_t bsize) if (bsize< tsize) goto out; >>> >>> + /* Pad table allocation for next/check by 256 entries to >>> remain + * backwards compatible with old (buggy) tools and >>> remain safe without + * run time checks + */ + if >>> (th.td_id == YYTD_ID_NXT || th.td_id == YYTD_ID_CHK) + >>> tsize += 256 * th.td_flags; + /* freed by free_table */ table = >>> kmalloc(tsize, GFP_KERNEL | __GFP_NOWARN); if (!table) { @@ >>> -178,7 +185,7 @@ static int verify_dfa(struct aa_dfa *dfa, int >>> flags) goto out; /* TODO: do check that DEF state recursion >>> terminates */ if (BASE_TABLE(dfa)[i] + 255>= trans_count) { - >>> printk("AppArmor DFA next/check upper bounds error\n"); + >>> AA_WARN("DFA next/check upper bounds fixed, upgrade user space >>> tools\n"); goto out; } } >> >> I'm having a hard time parsing the commit log message. Are you >> trying to say that the Lucid kernel won't work if the current >> Maverick apparmor user space is backported to Lucid? Confused... >> > no. I am saying there is a bug in the Lucid user space that will > cause policy loading to some times break if the previous patch 1 is > applied and either the Lucid userspace is not updated, or this patch > #2 is not applied > > The fix for Lucid userspace is in -proposed, and as long as it goes > to updates before a kernel with patch 1 hits -updates this patch is > NOT needed unless, we care about the odd situation where a user > installs an updated kernel but doesn't update their userspace. > > If the maverick userspace is used with the Lucid kernel with or > without these patches everything is fine. > Ah, so the aberrant case would be if a user is only subscribed to -security and then installs an LTS backport kernel? rtg
On 12/06/2010 12:53 PM, Tim Gardner wrote: > On 12/06/2010 01:49 PM, John Johansen wrote: >> On 12/06/2010 12:16 PM, Tim Gardner wrote: >>> On 12/03/2010 03:14 PM, John Johansen wrote: >>>> BugLink: http://bugs.launchpad.net/bugs/581525 >>>> >>>> SRU Justification: See bug link for full justification >>>> >>>> NOTE: This patch is OPTIONAL, and is only needed if the user >>>> space tools have not been updated. The userspace tools update >>>> has been pulled in, so unless there is the need to support users >>>> updating the kernel without updating the userspace tools, I am >>>> NOT recommending this patch for SRU, but including it for >>>> completeness. >>>> >>>> If the the patch to fix the bounds check on the next/check table >>>> has been applied, and the userspace has not been updated. The it >>>> is likely that part or all of policy will fail to load due to the >>>> bounds check rejecting policy. >>>> >>>> The Justification for this patch is that it supports using a >>>> non-updated (broken) userspace with a kernel that has the kernel >>>> fix for bug#581525 applied. >>>> >>>> The apparmor_parser when compiling policy could generate invalid >>>> dfas that did not have sufficient padding to avoid invalid >>>> references, when used by the kernel. The kernels check to verify >>>> the next/check table size was broken meaning invalid dfas were >>>> being created by userspace and not caught. >>>> >>>> To remain compatible with old tools that are not fixed, pad the >>>> loaded dfas next/check table. The dfa's themselves are valid >>>> except for the high padding for potentially invalid transitions >>>> (high bounds error), which have a maximimum is 256 entries. So >>>> just allocate an extra null filled 256 entries for the next/check >>>> tables. This will guarentee all bounds are good and invalid >>>> transitions go to the null (0) state. >>>> >>>> Signed-off-by: John Johansen<john.johansen@canonical.com> --- >>>> security/apparmor/include/apparmor.h | 6 ++++++ >>>> security/apparmor/match.c | 9 ++++++++- 2 files >>>> changed, 14 insertions(+), 1 deletions(-) >>>> >>>> diff --git a/security/apparmor/include/apparmor.h >>>> b/security/apparmor/include/apparmor.h index 3f8d866..b4ddc95 >>>> 100644 --- a/security/apparmor/include/apparmor.h +++ >>>> b/security/apparmor/include/apparmor.h @@ -46,6 +46,12 @@ extern >>>> unsigned int aa_g_path_max; printk(KERN_ERR "AppArmor: " fmt, >>>> ##args); \ } while (0) >>>> >>>> +#define AA_WARN(fmt, args...) \ + do { >>>> \ + if (printk_ratelimit()) \ + >>>> printk(KERN_WARN "AppArmor: " fmt, ##args); \ + } while >>>> (0) + /* Flag indicating whether initialization completed */ >>>> extern int apparmor_initialized; void apparmor_disable(void); >>>> diff --git a/security/apparmor/match.c >>>> b/security/apparmor/match.c index b456495..f4486c1 100644 --- >>>> a/security/apparmor/match.c +++ b/security/apparmor/match.c @@ >>>> -91,6 +91,13 @@ static struct table_header *unpack_table(char >>>> *blob, size_t bsize) if (bsize< tsize) goto out; >>>> >>>> + /* Pad table allocation for next/check by 256 entries to >>>> remain + * backwards compatible with old (buggy) tools and >>>> remain safe without + * run time checks + */ + if >>>> (th.td_id == YYTD_ID_NXT || th.td_id == YYTD_ID_CHK) + >>>> tsize += 256 * th.td_flags; + /* freed by free_table */ table = >>>> kmalloc(tsize, GFP_KERNEL | __GFP_NOWARN); if (!table) { @@ >>>> -178,7 +185,7 @@ static int verify_dfa(struct aa_dfa *dfa, int >>>> flags) goto out; /* TODO: do check that DEF state recursion >>>> terminates */ if (BASE_TABLE(dfa)[i] + 255>= trans_count) { - >>>> printk("AppArmor DFA next/check upper bounds error\n"); + >>>> AA_WARN("DFA next/check upper bounds fixed, upgrade user space >>>> tools\n"); goto out; } } >>> >>> I'm having a hard time parsing the commit log message. Are you >>> trying to say that the Lucid kernel won't work if the current >>> Maverick apparmor user space is backported to Lucid? Confused... >>> >> no. I am saying there is a bug in the Lucid user space that will >> cause policy loading to some times break if the previous patch 1 is >> applied and either the Lucid userspace is not updated, or this patch >> #2 is not applied >> >> The fix for Lucid userspace is in -proposed, and as long as it goes >> to updates before a kernel with patch 1 hits -updates this patch is >> NOT needed unless, we care about the odd situation where a user >> installs an updated kernel but doesn't update their userspace. >> >> If the maverick userspace is used with the Lucid kernel with or >> without these patches everything is fine. >> > > Ah, so the aberrant case would be if a user is only subscribed to -security and then installs an LTS backport kernel? > well no. Not as long as jdstrand rolls the userspace update through -security as we discussed. If that happens the only case is someone manually installing a kernel with the bound fix applied (via dpkg, or make) and they haven't updated their userspace via either -updates, -security, or manually. Its a case I don't believe we care about, and I would rather not have to carry a version of this patch in Natty and Natty+1, but if supporting this case is something we care about for the Lucid kernel then we will need it for Natty and and Natty+1 as well.
diff --git a/security/apparmor/include/apparmor.h b/security/apparmor/include/apparmor.h index 3f8d866..b4ddc95 100644 --- a/security/apparmor/include/apparmor.h +++ b/security/apparmor/include/apparmor.h @@ -46,6 +46,12 @@ extern unsigned int aa_g_path_max; printk(KERN_ERR "AppArmor: " fmt, ##args); \ } while (0) +#define AA_WARN(fmt, args...) \ + do { \ + if (printk_ratelimit()) \ + printk(KERN_WARN "AppArmor: " fmt, ##args); \ + } while (0) + /* Flag indicating whether initialization completed */ extern int apparmor_initialized; void apparmor_disable(void); diff --git a/security/apparmor/match.c b/security/apparmor/match.c index b456495..f4486c1 100644 --- a/security/apparmor/match.c +++ b/security/apparmor/match.c @@ -91,6 +91,13 @@ static struct table_header *unpack_table(char *blob, size_t bsize) if (bsize < tsize) goto out; + /* Pad table allocation for next/check by 256 entries to remain + * backwards compatible with old (buggy) tools and remain safe without + * run time checks + */ + if (th.td_id == YYTD_ID_NXT || th.td_id == YYTD_ID_CHK) + tsize += 256 * th.td_flags; + /* freed by free_table */ table = kmalloc(tsize, GFP_KERNEL | __GFP_NOWARN); if (!table) { @@ -178,7 +185,7 @@ static int verify_dfa(struct aa_dfa *dfa, int flags) goto out; /* TODO: do check that DEF state recursion terminates */ if (BASE_TABLE(dfa)[i] + 255 >= trans_count) { - printk("AppArmor DFA next/check upper bounds error\n"); + AA_WARN("DFA next/check upper bounds fixed, upgrade user space tools\n"); goto out; } }
BugLink: http://bugs.launchpad.net/bugs/581525 SRU Justification: See bug link for full justification NOTE: This patch is OPTIONAL, and is only needed if the user space tools have not been updated. The userspace tools update has been pulled in, so unless there is the need to support users updating the kernel without updating the userspace tools, I am NOT recommending this patch for SRU, but including it for completeness. If the the patch to fix the bounds check on the next/check table has been applied, and the userspace has not been updated. The it is likely that part or all of policy will fail to load due to the bounds check rejecting policy. The Justification for this patch is that it supports using a non-updated (broken) userspace with a kernel that has the kernel fix for bug#581525 applied. The apparmor_parser when compiling policy could generate invalid dfas that did not have sufficient padding to avoid invalid references, when used by the kernel. The kernels check to verify the next/check table size was broken meaning invalid dfas were being created by userspace and not caught. To remain compatible with old tools that are not fixed, pad the loaded dfas next/check table. The dfa's themselves are valid except for the high padding for potentially invalid transitions (high bounds error), which have a maximimum is 256 entries. So just allocate an extra null filled 256 entries for the next/check tables. This will guarentee all bounds are good and invalid transitions go to the null (0) state. Signed-off-by: John Johansen <john.johansen@canonical.com> --- security/apparmor/include/apparmor.h | 6 ++++++ security/apparmor/match.c | 9 ++++++++- 2 files changed, 14 insertions(+), 1 deletions(-)