George S. Coker, II
2007-Sep-27 16:43 UTC
[Xen-devel] [XSM:ACM][PATCH] null dereference bug fix
The attached patch fixes a null dereference bug in XSM:ACM. Signed-off-by: George Coker <gscoker@alpha.ncsc.mil> _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Stefan Berger
2007-Sep-27 18:35 UTC
Re: [Xense-devel] [XSM:ACM][PATCH] null dereference bug fix
xense-devel-bounces@lists.xensource.com wrote on 09/27/2007 12:43:35 PM:> The attached patch fixes a null dereference bug in XSM:ACM.As I read this in response to recent error reports - I wonder why CS 15661 does not expose this error whereas afterwards this error occurs. Are you sure this is the right solution? Was something changed in this area of the code between ''before XSM'' and afterwards? Stefan> > Signed-off-by: George Coker <gscoker@alpha.ncsc.mil> > [attachment "acm-xsm-null_bug-092707-xen-unstable-15880.diff" > deleted by Stefan Berger/Watson/IBM] > _______________________________________________ > Xense-devel mailing list > Xense-devel@lists.xensource.com > http://lists.xensource.com/xense-devel_______________________________________________ Xense-devel mailing list Xense-devel@lists.xensource.com http://lists.xensource.com/xense-devel
Coker, George
2007-Sep-27 19:35 UTC
Re: [Xen-devel] Re: [Xense-devel] [XSM:ACM][PATCH] nulldereference bug fix
This patch is correct for XSM. The patch creates clean acm_domain_create and acm_domain_destroy operations. In 15661 the logic under which acm_domain_destroy is called is slightly different than under XSM. In 15661, acm_domain_destroy is called only if the mask INIT_acm is set. INIT_acm is set only on successful return from acm_domain_create. When acm_domain_create fails, the mask is not set and acm_domain_destroy is not called. I do not know if this resulted in a leak in 15661 due to incomplete cleanup. George On Thu, 2007-09-27 at 14:35 -0400, Stefan Berger wrote:> > xense-devel-bounces@lists.xensource.com wrote on 09/27/2007 12:43:35 > PM: > > > The attached patch fixes a null dereference bug in XSM:ACM. > > As I read this in response to recent error reports - I wonder why CS > 15661 does not expose this error whereas afterwards this error occurs. > Are you sure this is the right solution? Was something changed in this > area of the code between ''before XSM'' and afterwards? > > Stefan > > > > > > > Signed-off-by: George Coker <gscoker@alpha.ncsc.mil> > > [attachment "acm-xsm-null_bug-092707-xen-unstable-15880.diff" > > deleted by Stefan Berger/Watson/IBM] > > _______________________________________________ > > Xense-devel mailing list > > Xense-devel@lists.xensource.com > > http://lists.xensource.com/xense-devel > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xensource.com > http://lists.xensource.com/xen-devel-- George S. Coker, II <gscoker@alpha.ncsc.mil> 443-479-6944 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Stefan Berger
2007-Sep-27 20:12 UTC
Re: [Xen-devel] Re: [Xense-devel] [XSM:ACM][PATCH] nulldereference bug fix
"Coker, George" <gscoker@tycho.ncsc.mil> wrote on 09/27/2007 03:35:14 PM:> This patch is correct for XSM. The patch creates clean > acm_domain_create and acm_domain_destroy operations. > > In 15661 the logic under which acm_domain_destroy is called is slightly > different than under XSM. In 15661, acm_domain_destroy is called only > if the mask INIT_acm is set. INIT_acm is set only on successful return > from acm_domain_create. When acm_domain_create fails, the mask is not > set and acm_domain_destroy is not called. I do not know if this > resulted in a leak in 15661 due to incomplete cleanup.So the roll-back call that was necessary before is not necessary anymore? static inline int acm_domain_create(struct domain *d, ssidref_t ssidref) { void *subject_ssid = current->domain->ssid; domid_t domid = d->domain_id; int rc; read_lock(&acm_bin_pol_rwlock); /* To be called when a domain is created; returns ''0'' if the domain is allowed to be created, != ''0'' if not. */ rc = acm_init_domain_ssid(d, ssidref); if (rc != ACM_OK) goto error_out; if ((acm_primary_ops->domain_create != NULL) && acm_primary_ops->domain_create(subject_ssid, ssidref, domid)) { rc = ACM_ACCESS_DENIED; } else if ((acm_secondary_ops->domain_create != NULL) && acm_secondary_ops->domain_create(subject_ssid, ssidref, domid)) { /* roll-back primary */ if (acm_primary_ops->domain_destroy != NULL) acm_primary_ops->domain_destroy(d->ssid, d); rc = ACM_ACCESS_DENIED; } if ( rc == ACM_OK ) { acm_domain_ssid_onto_list(d->ssid); } else { acm_free_domain_ssid(d->ssid); } error_out: read_unlock(&acm_bin_pol_rwlock); return rc; } The acm_primary_ops->domain_create() establishes state (see chwall_domain_create() in acm_chinesewall_hooks.c ) that if the secondary operation fails needs to be undone. That''s what the acm_primary_ops->domain_destroy() did, but you intend to remove it?! I have my doubts that this is correct. Which NULL pointer is the code running into and where? Stefan> > George > > > On Thu, 2007-09-27 at 14:35 -0400, Stefan Berger wrote: > > > > xense-devel-bounces@lists.xensource.com wrote on 09/27/2007 12:43:35 > > PM: > > > > > The attached patch fixes a null dereference bug in XSM:ACM. > > > > As I read this in response to recent error reports - I wonder why CS > > 15661 does not expose this error whereas afterwards this error occurs. > > Are you sure this is the right solution? Was something changed in this > > area of the code between ''before XSM'' and afterwards? > > > > Stefan > > > > > > > > > > > > Signed-off-by: George Coker <gscoker@alpha.ncsc.mil> > > > [attachment "acm-xsm-null_bug-092707-xen-unstable-15880.diff" > > > deleted by Stefan Berger/Watson/IBM] > > > _______________________________________________ > > > Xense-devel mailing list > > > Xense-devel@lists.xensource.com > > > http://lists.xensource.com/xense-devel > > _______________________________________________ > > Xen-devel mailing list > > Xen-devel@lists.xensource.com > > http://lists.xensource.com/xen-devel > -- > George S. Coker, II <gscoker@alpha.ncsc.mil> 443-479-6944_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
George S. Coker, II
2007-Sep-27 20:48 UTC
Re: [Xen-devel] Re: [Xense-devel] [XSM:ACM][PATCH] nulldereference bug fix
On Thu, 2007-09-27 at 16:12 -0400, Stefan Berger wrote:> > "Coker, George" <gscoker@tycho.ncsc.mil> wrote on 09/27/2007 03:35:14 > PM: > > > This patch is correct for XSM. The patch creates clean > > acm_domain_create and acm_domain_destroy operations. > > > > In 15661 the logic under which acm_domain_destroy is called is > slightly > > different than under XSM. In 15661, acm_domain_destroy is called > only > > if the mask INIT_acm is set. INIT_acm is set only on successful > return > > from acm_domain_create. When acm_domain_create fails, the mask is > not > > set and acm_domain_destroy is not called. I do not know if this > > resulted in a leak in 15661 due to incomplete cleanup. > > So the roll-back call that was necessary before is not necessary > anymore? >Yes, because on fail, acm_domain_create will always return ACM_ACCESS_DENIED which will cause domain_create to goto fail on return from xsm_domain_create. At fail, free_domain is called. free_domain calls xsm_free_security_domain which calls acm_domain_destroy. acm_domain_destroy calls the acm_primary_ops->domain_destroy followed by the acm_secondary_ops->domain_destroy. acm_domain_destroy ends with acm_free_domain_ssid. acm_domain_destroy already contains all of the rollback code that is replicated in acm_domain_create.> static inline int acm_domain_create(struct domain *d, ssidref_t > ssidref) > { > void *subject_ssid = current->domain->ssid; > domid_t domid = d->domain_id; > int rc; > > read_lock(&acm_bin_pol_rwlock); > /* > To be called when a domain is created; returns ''0'' if the > domain is allowed to be created, != ''0'' if not. > */ > rc = acm_init_domain_ssid(d, ssidref); > if (rc != ACM_OK) > goto error_out; > > if ((acm_primary_ops->domain_create != NULL) && > acm_primary_ops->domain_create(subject_ssid, ssidref, domid)) { > rc = ACM_ACCESS_DENIED; > } else if ((acm_secondary_ops->domain_create != NULL) && > acm_secondary_ops->domain_create(subject_ssid, ssidref, > domid)) { > /* roll-back primary */ > if (acm_primary_ops->domain_destroy != NULL) > acm_primary_ops->domain_destroy(d->ssid, d); > rc = ACM_ACCESS_DENIED; > } > > if ( rc == ACM_OK ) > { > acm_domain_ssid_onto_list(d->ssid); > } else { > acm_free_domain_ssid(d->ssid); > } > > error_out: > read_unlock(&acm_bin_pol_rwlock); > return rc; > } > > > The acm_primary_ops->domain_create() establishes state (see > chwall_domain_create() in acm_chinesewall_hooks.c ) that if the > secondary operation fails needs to be undone. That''s what the > acm_primary_ops->domain_destroy() did, but you intend to remove it?! I > have my doubts that this is correct. >Yes I am removing the rollback from acm_domain_create because it is already duplicated in acm_domain_destroy. acm_domain_destroy is always now called on fail under XSM. In 15661, acm_domain_destroy was not always called.> Which NULL pointer is the code running into and where?The NULL pointer was created in acm_free_domain_ssid and dereferenced in the fail code path in the call to xsm_free_security_domain in free_domain.> > Stefan > > > > > > George > > > > > > On Thu, 2007-09-27 at 14:35 -0400, Stefan Berger wrote: > > > > > > xense-devel-bounces@lists.xensource.com wrote on 09/27/2007 > 12:43:35 > > > PM: > > > > > > > The attached patch fixes a null dereference bug in XSM:ACM. > > > > > > As I read this in response to recent error reports - I wonder why > CS > > > 15661 does not expose this error whereas afterwards this error > occurs. > > > Are you sure this is the right solution? Was something changed in > this > > > area of the code between ''before XSM'' and afterwards? > > > > > > Stefan > > > > > > > > > > > > > > > > > Signed-off-by: George Coker <gscoker@alpha.ncsc.mil> > > > > [attachment "acm-xsm-null_bug-092707-xen-unstable-15880.diff" > > > > deleted by Stefan Berger/Watson/IBM] > > > > _______________________________________________ > > > > Xense-devel mailing list > > > > Xense-devel@lists.xensource.com > > > > http://lists.xensource.com/xense-devel > > > _______________________________________________ > > > Xen-devel mailing list > > > Xen-devel@lists.xensource.com > > > http://lists.xensource.com/xen-devel > > -- > > George S. Coker, II <gscoker@alpha.ncsc.mil> 443-479-6944 > _______________________________________________ > Xense-devel mailing list > Xense-devel@lists.xensource.com > http://lists.xensource.com/xense-devel-- George S. Coker, II <gscoker@alpha.ncsc.mil> 443-479-6944 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Syunsuke HAYASHI
2007-Sep-28 07:56 UTC
Re: [Xen-devel] Re: [Xense-devel] [XSM:ACM][PATCH] nulldereference bug fix
Hi, I tested your patch. I confirmed that XSM/ACM run normally. Thank you for your help. The result is put as follows. #xm create vm1.conf <---- create domain1 Using config file "./vm1.conf". #xm list --label Name ID Mem VCPUs State Time(s) Label vm1 3 128 1 r----- 1.0 ACM:example.client_v1:dom_HomeBanking Domain-0 0 743 2 r----- 0.0 ACM:example.client_v1:dom_SystemManagement #xm create vm2.conf <---- create domain2 Using config file "./vm2.conf". Internal error: Domain in conflict set with running domain?. ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ #xm list --label Name ID Mem VCPUs State Time(s) Label vm1 3 128 1 r----- 1.0 ACM:example.client_v1:dom_HomeBanking Domain-0 0 743 2 r----- 0.0 ACM:example.client_v1:dom_SystemManagement Chinese Wall looks good and "system boot" did not appen. Syunsuke HAYASHI.> On Thu, 2007-09-27 at 16:12 -0400, Stefan Berger wrote: >> "Coker, George" <gscoker@tycho.ncsc.mil> wrote on 09/27/2007 03:35:14 >> PM: >> >>> This patch is correct for XSM. The patch creates clean >>> acm_domain_create and acm_domain_destroy operations. >>> >>> In 15661 the logic under which acm_domain_destroy is called is >> slightly >>> different than under XSM. In 15661, acm_domain_destroy is called >> only >>> if the mask INIT_acm is set. INIT_acm is set only on successful >> return >>> from acm_domain_create. When acm_domain_create fails, the mask is >> not >>> set and acm_domain_destroy is not called. I do not know if this >>> resulted in a leak in 15661 due to incomplete cleanup. >> So the roll-back call that was necessary before is not necessary >> anymore? >> > > Yes, because on fail, acm_domain_create will always return > ACM_ACCESS_DENIED which will cause domain_create to goto fail on return > from xsm_domain_create. At fail, free_domain is called. free_domain > calls xsm_free_security_domain which calls acm_domain_destroy. > > acm_domain_destroy calls the acm_primary_ops->domain_destroy followed by > the acm_secondary_ops->domain_destroy. acm_domain_destroy ends with > acm_free_domain_ssid. acm_domain_destroy already contains all of the > rollback code that is replicated in acm_domain_create. > >> static inline int acm_domain_create(struct domain *d, ssidref_t >> ssidref) >> { >> void *subject_ssid = current->domain->ssid; >> domid_t domid = d->domain_id; >> int rc; >> >> read_lock(&acm_bin_pol_rwlock); >> /* >> To be called when a domain is created; returns ''0'' if the >> domain is allowed to be created, != ''0'' if not. >> */ >> rc = acm_init_domain_ssid(d, ssidref); >> if (rc != ACM_OK) >> goto error_out; >> >> if ((acm_primary_ops->domain_create != NULL) && >> acm_primary_ops->domain_create(subject_ssid, ssidref, domid)) { >> rc = ACM_ACCESS_DENIED; >> } else if ((acm_secondary_ops->domain_create != NULL) && >> acm_secondary_ops->domain_create(subject_ssid, ssidref, >> domid)) { >> /* roll-back primary */ >> if (acm_primary_ops->domain_destroy != NULL) >> acm_primary_ops->domain_destroy(d->ssid, d); >> rc = ACM_ACCESS_DENIED; >> } >> >> if ( rc == ACM_OK ) >> { >> acm_domain_ssid_onto_list(d->ssid); >> } else { >> acm_free_domain_ssid(d->ssid); >> } >> >> error_out: >> read_unlock(&acm_bin_pol_rwlock); >> return rc; >> } >> >> >> The acm_primary_ops->domain_create() establishes state (see >> chwall_domain_create() in acm_chinesewall_hooks.c ) that if the >> secondary operation fails needs to be undone. That''s what the >> acm_primary_ops->domain_destroy() did, but you intend to remove it?! I >> have my doubts that this is correct. >> > Yes I am removing the rollback from acm_domain_create because it is > already duplicated in acm_domain_destroy. acm_domain_destroy is always > now called on fail under XSM. In 15661, acm_domain_destroy was not > always called. > >> Which NULL pointer is the code running into and where? > > The NULL pointer was created in acm_free_domain_ssid and dereferenced in > the fail code path in the call to xsm_free_security_domain in > free_domain. > >> Stefan >> >> >>> George >>> >>> >>> On Thu, 2007-09-27 at 14:35 -0400, Stefan Berger wrote: >>>> xense-devel-bounces@lists.xensource.com wrote on 09/27/2007 >> 12:43:35 >>>> PM: >>>> >>>>> The attached patch fixes a null dereference bug in XSM:ACM. >>>> As I read this in response to recent error reports - I wonder why >> CS >>>> 15661 does not expose this error whereas afterwards this error >> occurs. >>>> Are you sure this is the right solution? Was something changed in >> this >>>> area of the code between ''before XSM'' and afterwards? >>>> >>>> Stefan >>>> >>>> >>>> >>>>> Signed-off-by: George Coker <gscoker@alpha.ncsc.mil> >>>>> [attachment "acm-xsm-null_bug-092707-xen-unstable-15880.diff" >>>>> deleted by Stefan Berger/Watson/IBM] >>>>> _______________________________________________ >>>>> Xense-devel mailing list >>>>> Xense-devel@lists.xensource.com >>>>> http://lists.xensource.com/xense-devel >>>> _______________________________________________ >>>> Xen-devel mailing list >>>> Xen-devel@lists.xensource.com >>>> http://lists.xensource.com/xen-devel >>> -- >>> George S. Coker, II <gscoker@alpha.ncsc.mil> 443-479-6944 >> _______________________________________________ >> Xense-devel mailing list >> Xense-devel@lists.xensource.com >> http://lists.xensource.com/xense-devel-- ///////////////////////////////////////////// 富士通株式会社 新横浜TECHビル A館6F サーバシステム事業本部 Linux技術開発統括部 Name : Syunsuke HAYASHI (林 俊介) TEL : 7124-3846(内線) 045-473-9478(外線) E-mail : syunsuke@jp.fujitsu.com //////////////////////////////////////////// _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Stefan Berger
2007-Sep-28 14:21 UTC
Re: [Xen-devel] Re: [Xense-devel] [XSM:ACM][PATCH] nulldereference bug fix
xen-devel-bounces@lists.xensource.com wrote on 09/27/2007 04:48:14 PM:> On Thu, 2007-09-27 at 16:12 -0400, Stefan Berger wrote: > > > > "Coker, George" <gscoker@tycho.ncsc.mil> wrote on 09/27/2007 03:35:14 > > PM: > > > > > This patch is correct for XSM. The patch creates clean > > > acm_domain_create and acm_domain_destroy operations. > > > > > > In 15661 the logic under which acm_domain_destroy is called is > > slightly > > > different than under XSM. In 15661, acm_domain_destroy is called > > only > > > if the mask INIT_acm is set. INIT_acm is set only on successful > > return > > > from acm_domain_create. When acm_domain_create fails, the mask is > > not > > > set and acm_domain_destroy is not called. I do not know if this > > > resulted in a leak in 15661 due to incomplete cleanup. > > > > So the roll-back call that was necessary before is not necessary > > anymore? > > > > Yes, because on fail, acm_domain_create will always return > ACM_ACCESS_DENIED which will cause domain_create to goto fail on return > from xsm_domain_create. At fail, free_domain is called. free_domain > calls xsm_free_security_domain which calls acm_domain_destroy.The problem is that the roll-back related to chinese wall must *only* be done when the check on the chinese wall was successful and NOT when it was not successful. Following the test Syunsuke HAYASHI describes in http://lists.xensource.com/archives/html/xen-devel/2007-09/msg00514.html I get the following after creating the 1st domain when doing an ''xm dumppolicy'': Policy dump: ===========POLICY REFERENCE = example.client_v1. PolicyVer = 8c000000. XML Vers. = 1.24 Magic = 1debc. Len = 178. Primary = CHINESE WALL (c=1, off=40). Secondary = SIMPLE TYPE ENFORCEMENT (c=2, off=b8). Chinese Wall policy: ===================Policy version= ffffe849. Max Types = 4. Max Ssidrefs = 7. Max ConfSets = 1. Ssidrefs Off = 24. Conflicts Off = 5c. Runing T. Off = 64. C. Agg. Off = 6c. SSID To CHWALL-Type matrix: ssidref 0: 00 00 00 00 ssidref 1: 00 00 00 01 <-- Domain-0 ssidref 2: 00 01 00 00 ssidref 3: 01 00 00 00 ssidref 4: 00 00 01 00 ssidref 5: 00 00 00 01 ssidref 6: 00 00 00 01 Confict Sets: c-set 0: 01 00 01 00 Running Types: 00 00 01 01 Conflict Aggregate Set: 01 00 00 00 Simple Type Enforcement policy: ==============================Policy version= cef6c202. Max Types = 6. Max Ssidrefs = e. Ssidrefs Off = 14. SSID To STE-Type matrix: ssidref 0: 00 00 00 00 00 00 ssidref 1: 01 01 01 01 01 01 <-- Domain-0 ssidref 2: 01 00 00 00 00 00 ssidref 3: 00 01 00 00 00 00 ssidref 4: 00 00 00 00 01 00 ssidref 5: 01 01 01 00 01 00 ssidref 6: 00 01 00 01 01 00 ssidref 7: 00 00 01 00 00 00 ssidref 8: 00 00 00 00 00 01 ssidref 9: 00 00 00 01 00 00 ssidref a: 00 00 00 00 01 00 ssidref b: 00 01 00 00 00 00 ssidref c: 00 00 00 00 00 01 ssidref d: 00 00 00 00 01 00 This is output is correct. After trying to start the 2nd domain I now get: Policy dump: ===========POLICY REFERENCE = example.client_v1. PolicyVer = 0. XML Vers. = 1.24 Magic = 1debc. Len = 178. Primary = CHINESE WALL (c=1, off=40). Secondary = SIMPLE TYPE ENFORCEMENT (c=2, off=b8). Chinese Wall policy: ===================Policy version= 100ff00. Max Types = 4. Max Ssidrefs = 7. Max ConfSets = 1. Ssidrefs Off = 24. Conflicts Off = 5c. Runing T. Off = 64. C. Agg. Off = 6c. SSID To CHWALL-Type matrix: ssidref 0: 00 00 00 00 ssidref 1: 00 00 00 01 <-- Domain-0 ssidref 2: 00 01 00 00 ssidref 3: 01 00 00 00 ssidref 4: 00 00 01 00 ssidref 5: 00 00 00 01 ssidref 6: 00 00 00 01 Confict Sets: c-set 0: 01 00 01 00 Running Types: ffff 00 01 01 Conflict Aggregate Set: 01 00 ffff 00 Simple Type Enforcement policy: ==============================Policy version= 0. Max Types = 6. Max Ssidrefs = e. Ssidrefs Off = 14. SSID To STE-Type matrix: ssidref 0: 00 00 00 00 00 00 ssidref 1: 01 01 01 01 01 01 <-- Domain-0 ssidref 2: 01 00 00 00 00 00 ssidref 3: 00 01 00 00 00 00 ssidref 4: 00 00 00 00 01 00 ssidref 5: 01 01 01 00 01 00 ssidref 6: 00 01 00 01 01 00 ssidref 7: 00 00 01 00 00 00 ssidref 8: 00 00 00 00 00 01 ssidref 9: 00 00 00 01 00 00 ssidref a: 00 00 00 00 01 00 ssidref b: 00 01 00 00 00 00 ssidref c: 00 00 00 00 00 01 ssidref d: 00 00 00 00 01 00 Obviously the ''Running Types'' and ''Conflict Aggregate Set'' are showing wrong numbers due to the state on the chinese wall having been rolled back, although it should not have been. Also, the reason why this operation was protected through the surrounding lock is that while this test happens no policy change may occur, which would recalculate all the state. So I''d rather have this unrolling left where it was. Stefan> > acm_domain_destroy calls the acm_primary_ops->domain_destroy followed by > the acm_secondary_ops->domain_destroy. acm_domain_destroy ends with > acm_free_domain_ssid. acm_domain_destroy already contains all of the > rollback code that is replicated in acm_domain_create. > > > static inline int acm_domain_create(struct domain *d, ssidref_t > > ssidref) > > { > > void *subject_ssid = current->domain->ssid; > > domid_t domid = d->domain_id; > > int rc; > > > > read_lock(&acm_bin_pol_rwlock); > > /* > > To be called when a domain is created; returns ''0'' if the > > domain is allowed to be created, != ''0'' if not. > > */ > > rc = acm_init_domain_ssid(d, ssidref); > > if (rc != ACM_OK) > > goto error_out; > > > > if ((acm_primary_ops->domain_create != NULL) && > > acm_primary_ops->domain_create(subject_ssid, ssidref, domid)) { > > rc = ACM_ACCESS_DENIED; > > } else if ((acm_secondary_ops->domain_create != NULL) && > > acm_secondary_ops->domain_create(subject_ssid, ssidref, > > domid)) { > > /* roll-back primary */ > > if (acm_primary_ops->domain_destroy != NULL) > > acm_primary_ops->domain_destroy(d->ssid, d); > > rc = ACM_ACCESS_DENIED; > > } > > > > if ( rc == ACM_OK ) > > { > > acm_domain_ssid_onto_list(d->ssid); > > } else { > > acm_free_domain_ssid(d->ssid); > > } > > > > error_out: > > read_unlock(&acm_bin_pol_rwlock); > > return rc; > > } > > > > > > The acm_primary_ops->domain_create() establishes state (see > > chwall_domain_create() in acm_chinesewall_hooks.c ) that if the > > secondary operation fails needs to be undone. That''s what the > > acm_primary_ops->domain_destroy() did, but you intend to remove it?! I > > have my doubts that this is correct. > > > Yes I am removing the rollback from acm_domain_create because it is > already duplicated in acm_domain_destroy. acm_domain_destroy is always > now called on fail under XSM. In 15661, acm_domain_destroy was not > always called. > > > Which NULL pointer is the code running into and where? > > The NULL pointer was created in acm_free_domain_ssid and dereferenced in > the fail code path in the call to xsm_free_security_domain in > free_domain. > > > > > Stefan > > > > > > > > > > George > > > > > > > > > On Thu, 2007-09-27 at 14:35 -0400, Stefan Berger wrote: > > > > > > > > xense-devel-bounces@lists.xensource.com wrote on 09/27/2007 > > 12:43:35 > > > > PM: > > > > > > > > > The attached patch fixes a null dereference bug in XSM:ACM. > > > > > > > > As I read this in response to recent error reports - I wonder why > > CS > > > > 15661 does not expose this error whereas afterwards this error > > occurs. > > > > Are you sure this is the right solution? Was something changed in > > this > > > > area of the code between ''before XSM'' and afterwards? > > > > > > > > Stefan > > > > > > > > > > > > > > > > > > > > > > Signed-off-by: George Coker <gscoker@alpha.ncsc.mil> > > > > > [attachment "acm-xsm-null_bug-092707-xen-unstable-15880.diff" > > > > > deleted by Stefan Berger/Watson/IBM] > > > > > _______________________________________________ > > > > > Xense-devel mailing list > > > > > Xense-devel@lists.xensource.com > > > > > http://lists.xensource.com/xense-devel > > > > _______________________________________________ > > > > Xen-devel mailing list > > > > Xen-devel@lists.xensource.com > > > > http://lists.xensource.com/xen-devel > > > -- > > > George S. Coker, II <gscoker@alpha.ncsc.mil> 443-479-6944 > > _______________________________________________ > > Xense-devel mailing list > > Xense-devel@lists.xensource.com > > http://lists.xensource.com/xense-devel > -- > George S. Coker, II <gscoker@alpha.ncsc.mil> 443-479-6944 > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xensource.com > http://lists.xensource.com/xen-devel_______________________________________________ Xense-devel mailing list Xense-devel@lists.xensource.com http://lists.xensource.com/xense-devel
George S. Coker, II
2007-Sep-28 15:05 UTC
Re: [Xen-devel] Re: [Xense-devel] [XSM:ACM][PATCH] nulldereference bug fix
On Fri, 2007-09-28 at 10:21 -0400, Stefan Berger wrote:> The problem is that the roll-back related to chinese wall must *only* > be done when the check on the chinese wall was successful and NOT when > it was not successful. > Following the test Syunsuke HAYASHI describes in > > http://lists.xensource.com/archives/html/xen-devel/2007-09/msg00514.html > > > I get the following after creating the 1st domain when doing an ''xm > dumppolicy'': > > > > Policy dump: > ===========> POLICY REFERENCE = example.client_v1. > PolicyVer = 8c000000. > XML Vers. = 1.24 > Magic = 1debc. > Len = 178. > Primary = CHINESE WALL (c=1, off=40). > Secondary = SIMPLE TYPE ENFORCEMENT (c=2, off=b8). > > > Chinese Wall policy: > ===================> Policy version= ffffe849. > Max Types = 4. > Max Ssidrefs = 7. > Max ConfSets = 1. > Ssidrefs Off = 24. > Conflicts Off = 5c. > Runing T. Off = 64. > C. Agg. Off = 6c. > > SSID To CHWALL-Type matrix: > > ssidref 0: 00 00 00 00 > ssidref 1: 00 00 00 01 <-- Domain-0 > ssidref 2: 00 01 00 00 > ssidref 3: 01 00 00 00 > ssidref 4: 00 00 01 00 > ssidref 5: 00 00 00 01 > ssidref 6: 00 00 00 01 > > Confict Sets: > > c-set 0: 01 00 01 00 > > Running > Types: 00 00 01 01 > > Conflict > Aggregate Set: 01 00 00 00 > > > Simple Type Enforcement policy: > ==============================> Policy version= cef6c202. > Max Types = 6. > Max Ssidrefs = e. > Ssidrefs Off = 14. > > SSID To STE-Type matrix: > > ssidref 0: 00 00 00 00 00 00 > ssidref 1: 01 01 01 01 01 01 <-- Domain-0 > ssidref 2: 01 00 00 00 00 00 > ssidref 3: 00 01 00 00 00 00 > ssidref 4: 00 00 00 00 01 00 > ssidref 5: 01 01 01 00 01 00 > ssidref 6: 00 01 00 01 01 00 > ssidref 7: 00 00 01 00 00 00 > ssidref 8: 00 00 00 00 00 01 > ssidref 9: 00 00 00 01 00 00 > ssidref a: 00 00 00 00 01 00 > ssidref b: 00 01 00 00 00 00 > ssidref c: 00 00 00 00 00 01 > ssidref d: 00 00 00 00 01 00 > > > > > This is output is correct. > > > After trying to start the 2nd domain I now get: > > > > Policy dump: > ===========> POLICY REFERENCE = example.client_v1. > PolicyVer = 0. > XML Vers. = 1.24 > Magic = 1debc. > Len = 178. > Primary = CHINESE WALL (c=1, off=40). > Secondary = SIMPLE TYPE ENFORCEMENT (c=2, off=b8). > > > Chinese Wall policy: > ===================> Policy version= 100ff00. > Max Types = 4. > Max Ssidrefs = 7. > Max ConfSets = 1. > Ssidrefs Off = 24. > Conflicts Off = 5c. > Runing T. Off = 64. > C. Agg. Off = 6c. > > SSID To CHWALL-Type matrix: > > ssidref 0: 00 00 00 00 > ssidref 1: 00 00 00 01 <-- Domain-0 > ssidref 2: 00 01 00 00 > ssidref 3: 01 00 00 00 > ssidref 4: 00 00 01 00 > ssidref 5: 00 00 00 01 > ssidref 6: 00 00 00 01 > > Confict Sets: > > c-set 0: 01 00 01 00 > > Running > Types: ffff 00 01 01 > > Conflict > Aggregate Set: 01 00 ffff 00 > > > Simple Type Enforcement policy: > ==============================> Policy version= 0. > Max Types = 6. > Max Ssidrefs = e. > Ssidrefs Off = 14. > > SSID To STE-Type matrix: > > ssidref 0: 00 00 00 00 00 00 > ssidref 1: 01 01 01 01 01 01 <-- Domain-0 > ssidref 2: 01 00 00 00 00 00 > ssidref 3: 00 01 00 00 00 00 > ssidref 4: 00 00 00 00 01 00 > ssidref 5: 01 01 01 00 01 00 > ssidref 6: 00 01 00 01 01 00 > ssidref 7: 00 00 01 00 00 00 > ssidref 8: 00 00 00 00 00 01 > ssidref 9: 00 00 00 01 00 00 > ssidref a: 00 00 00 00 01 00 > ssidref b: 00 01 00 00 00 00 > ssidref c: 00 00 00 00 00 01 > ssidref d: 00 00 00 00 01 00 > > > > Obviously the ''Running Types'' and ''Conflict Aggregate Set'' are showing > wrong numbers due to the state on the chinese wall having been rolled > back, although it should not have been. Also, the reason why this > operation was protected through the surrounding lock is that while > this test happens no policy change may occur, which would recalculate > all the state. So I''d rather have this unrolling left where it was. >> > > > > static inline int acm_domain_create(struct domain *d, ssidref_t > > > ssidref) > > > { > > > void *subject_ssid = current->domain->ssid; > > > domid_t domid = d->domain_id; > > > int rc; > > > > > > read_lock(&acm_bin_pol_rwlock); > > > /* > > > To be called when a domain is created; returns ''0'' if the > > > domain is allowed to be created, != ''0'' if not. > > > */ > > > rc = acm_init_domain_ssid(d, ssidref); > > > if (rc != ACM_OK) > > > goto error_out; > > > > > > if ((acm_primary_ops->domain_create != NULL) && > > > acm_primary_ops->domain_create(subject_ssid, ssidref, > domid)) { > > > rc = ACM_ACCESS_DENIED; > > > } else if ((acm_secondary_ops->domain_create != NULL) && > > > acm_secondary_ops->domain_create(subject_ssid, > ssidref, > > > domid)) { > > > /* roll-back primary */ > > > if (acm_primary_ops->domain_destroy != NULL) > > > acm_primary_ops->domain_destroy(d->ssid, d); > > > rc = ACM_ACCESS_DENIED; > > > } > > > > > > if ( rc == ACM_OK ) > > > { > > > acm_domain_ssid_onto_list(d->ssid); > > > } else { > > > acm_free_domain_ssid(d->ssid); > > > } > > > > > > error_out: > > > read_unlock(&acm_bin_pol_rwlock); > > > return rc; > > > } > > >Ok, but then the null dereference issue still exists within the context of XSM. The options are: 1) Follow acm_free_domain_ssid(d->ssid) with d->ssid = NULL (I''ve tested this and it maintains the desired policy state: Confict Sets: c-set 0: 00 01 00 01 Running Types: 01 01 00 00 Conflict Aggregate Set: 00 00 00 01 but seems less attractive from a coding style perspective) or 2) Chance acm_free_domain_ssid to pass d instead of ssid and ensure that d->ssid = NULL in acm_free_domain_ssid. George _______________________________________________ Xense-devel mailing list Xense-devel@lists.xensource.com http://lists.xensource.com/xense-devel
Stefan Berger
2007-Sep-28 15:20 UTC
Re: [Xen-devel] Re: [Xense-devel] [XSM:ACM][PATCH] nulldereference bug fix
[...]> > > > > > Ok, but then the null dereference issue still exists within the context > of XSM. The options are: > > 1) Follow acm_free_domain_ssid(d->ssid) with d->ssid = NULL > (I''ve tested this and it maintains the desired policy state: > > Confict Sets: > > c-set 0: 00 01 00 01 > > Running > Types: 01 01 00 00 > > Conflict > Aggregate Set: 00 00 00 01 > > but seems less attractive from a coding style perspective) > > or > > 2) Chance acm_free_domain_ssid to pass d instead of ssid and ensure that > d->ssid = NULL in acm_free_domain_ssid.Sounds like this is cleaner and so I opt for this choice. Stefan> > George_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Coker, George
2007-Sep-28 16:04 UTC
Re: [Xen-devel] Re: [Xense-devel][XSM:ACM][PATCH] nulldereference bug fix
On Fri, 2007-09-28 at 11:20 -0400, Stefan Berger wrote:> > [...] > > > > > > > > > Ok, but then the null dereference issue still exists within the > context > > of XSM. The options are: > > > > 1) Follow acm_free_domain_ssid(d->ssid) with d->ssid = NULL > > (I''ve tested this and it maintains the desired policy state: > > > > Confict Sets: > > > > c-set 0: 00 01 00 01 > > > > Running > > Types: 01 01 00 00 > > > > Conflict > > Aggregate Set: 00 00 00 01 > > > > but seems less attractive from a coding style perspective) > > > > or > > > > 2) Chance acm_free_domain_ssid to pass d instead of ssid and ensure > that > > d->ssid = NULL in acm_free_domain_ssid. > > Sounds like this is cleaner and so I opt for this choice. >I''ve just posted a patch. Would you care to check it out and Ack it to avoid any confusion? George _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel