Re: [Xen-devel] Re: [Xense-devel] [XSM:ACM][PATCH] nulldereference bug fix

[Stefan Berger <stefanb@xxxxxxxxxx>]

xen-devel-bounces@xxxxxxxxxxxxxxxxxxx wrote on 09/27/2007
04:48:14 PM:

> On Thu, 2007-09-27 at 16:12 -0400, Stefan Berger wrote:
> > 
> > "Coker, George" <gscoker@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxx 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@xxxxxxxxxxxxxx>
> > > > > [attachment "acm-xsm-null_bug-092707-xen-unstable-15880.diff"
> > > > > deleted by Stefan Berger/Watson/IBM] 
> > > > > _______________________________________________
> > > > > Xense-devel mailing list
> > > > > Xense-devel@xxxxxxxxxxxxxxxxxxx
> > > > > http://lists.xensource.com/xense-devel
> > > > _______________________________________________
> > > > Xen-devel mailing list
> > > > Xen-devel@xxxxxxxxxxxxxxxxxxx
> > > > http://lists.xensource.com/xen-devel
> > > -- 
> > > George S. Coker, II <gscoker@xxxxxxxxxxxxxx> 443-479-6944
> > _______________________________________________
> > Xense-devel mailing list
> > Xense-devel@xxxxxxxxxxxxxxxxxxx
> > http://lists.xensource.com/xense-devel
> -- 
> George S. Coker, II <gscoker@xxxxxxxxxxxxxx> 443-479-6944
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@xxxxxxxxxxxxxxxxxxx
> http://lists.xensource.com/xen-devel
_______________________________________________
Xense-devel mailing list
Xense-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xense-devel