| Message ID | 20191004164243.30822-1-julien.grall@arm.com |
|---|---|
| State | New |
| Headers | show |
| Series | [Xen-devel,for-4.13] xen/xsm: flask: Prevent NULL deference in flask_assign_{, dt}device() | expand |
On 10/4/19 12:42 PM, Julien Grall wrote: > flask_assign_{, dt}device() may be used to check whether you can test if > a device is assigned. In this case, the domain will be NULL. > > However, flask_iommu_resource_use_perm() will be called and may end up > to deference a NULL pointer. This can be prevented by moving the call > after we check the validity for the domain pointer. > > Coverity-ID: 1486741 > Fixes: 71e617a6b8 ('use is_iommu_enabled() where appropriate...') > Signed-off-by: Julien Grall <julien.grall@arm.com> Acked-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
On Fri, 4 Oct 2019 at 17:42, Julien Grall <julien.grall@arm.com> wrote: > > flask_assign_{, dt}device() may be used to check whether you can test if > a device is assigned. In this case, the domain will be NULL. > > However, flask_iommu_resource_use_perm() will be called and may end up > to deference a NULL pointer. This can be prevented by moving the call > after we check the validity for the domain pointer. > > Coverity-ID: 1486741 > Fixes: 71e617a6b8 ('use is_iommu_enabled() where appropriate...') > Signed-off-by: Julien Grall <julien.grall@arm.com> Reviewed-by: Paul Durrant <paul@xen.org> > --- > xen/xsm/flask/hooks.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c > index 3b30827764..cf7f25cda2 100644 > --- a/xen/xsm/flask/hooks.c > +++ b/xen/xsm/flask/hooks.c > @@ -1296,11 +1296,13 @@ static int flask_assign_device(struct domain *d, uint32_t machine_bdf) > u32 dsid, rsid; > int rc = -EPERM; > struct avc_audit_data ad; > - u32 dperm = flask_iommu_resource_use_perm(d); > + u32 dperm; > > if ( !d ) > return flask_test_assign_device(machine_bdf); > > + dperm = flask_iommu_resource_use_perm(d); > + > rc = current_has_perm(d, SECCLASS_RESOURCE, RESOURCE__ADD); > if ( rc ) > return rc; > @@ -1355,11 +1357,13 @@ static int flask_assign_dtdevice(struct domain *d, const char *dtpath) > u32 dsid, rsid; > int rc = -EPERM; > struct avc_audit_data ad; > - u32 dperm = flask_iommu_resource_use_perm(d); > + u32 dperm; > > if ( !d ) > return flask_test_assign_dtdevice(dtpath); > > + dperm = flask_iommu_resource_use_perm(d); > + > rc = current_has_perm(d, SECCLASS_RESOURCE, RESOURCE__ADD); > if ( rc ) > return rc; > -- > 2.11.0 >
On 04.10.19 18:42, Julien Grall wrote: > flask_assign_{, dt}device() may be used to check whether you can test if > a device is assigned. In this case, the domain will be NULL. > > However, flask_iommu_resource_use_perm() will be called and may end up > to deference a NULL pointer. This can be prevented by moving the call > after we check the validity for the domain pointer. > > Coverity-ID: 1486741 > Fixes: 71e617a6b8 ('use is_iommu_enabled() where appropriate...') > Signed-off-by: Julien Grall <julien.grall@arm.com> Release-acked-by: Juergen Gross <jgross@suse.com> Juergen
Hi Julien On Fri, 2019-10-04 at 17:42 +0100, Julien Grall wrote: > flask_assign_{, dt}device() may be used to check whether you can test > if > a device is assigned. In this case, the domain will be NULL. > > However, flask_iommu_resource_use_perm() will be called and may end > up > to deference a NULL pointer. This can be prevented by moving the call > after we check the validity for the domain pointer. > > Coverity-ID: 1486741 The correct CID is 1486742 > Fixes: 71e617a6b8 ('use is_iommu_enabled() where appropriate...') > Signed-off-by: Julien Grall < > julien.grall@arm.com > > > --- > xen/xsm/flask/hooks.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c > index 3b30827764..cf7f25cda2 100644 > --- a/xen/xsm/flask/hooks.c > +++ b/xen/xsm/flask/hooks.c > @@ -1296,11 +1296,13 @@ static int flask_assign_device(struct domain > *d, uint32_t machine_bdf) > u32 dsid, rsid; > int rc = -EPERM; > struct avc_audit_data ad; > - u32 dperm = flask_iommu_resource_use_perm(d); > + u32 dperm; > > if ( !d ) > return flask_test_assign_device(machine_bdf); > > + dperm = flask_iommu_resource_use_perm(d); > + > rc = current_has_perm(d, SECCLASS_RESOURCE, RESOURCE__ADD); > if ( rc ) > return rc; > @@ -1355,11 +1357,13 @@ static int flask_assign_dtdevice(struct > domain *d, const char *dtpath) > u32 dsid, rsid; > int rc = -EPERM; > struct avc_audit_data ad; > - u32 dperm = flask_iommu_resource_use_perm(d); > + u32 dperm; > > if ( !d ) > return flask_test_assign_dtdevice(dtpath); > > + dperm = flask_iommu_resource_use_perm(d); > + > rc = current_has_perm(d, SECCLASS_RESOURCE, RESOURCE__ADD); > if ( rc ) > return rc;
On 09/10/2019 12:57, Artem Mygaiev wrote: > Hi Julien Hi, > On Fri, 2019-10-04 at 17:42 +0100, Julien Grall wrote: >> flask_assign_{, dt}device() may be used to check whether you can test >> if >> a device is assigned. In this case, the domain will be NULL. >> >> However, flask_iommu_resource_use_perm() will be called and may end >> up >> to deference a NULL pointer. This can be prevented by moving the call >> after we check the validity for the domain pointer. >> >> Coverity-ID: 1486741 > > The correct CID is 1486742 Hmmm yes. The coverity report e-mail is a bit confusing to read. However, I have already committed the patch so we will have to leave with it :(. Cheers,
Hi, On Thu, 2019-10-10 at 15:48 +0100, Julien Grall wrote: > > On 09/10/2019 12:57, Artem Mygaiev wrote: > > Hi Julien > > Hi, > > > On Fri, 2019-10-04 at 17:42 +0100, Julien Grall wrote: > > > flask_assign_{, dt}device() may be used to check whether you can test > > > if > > > a device is assigned. In this case, the domain will be NULL. > > > > > > However, flask_iommu_resource_use_perm() will be called and may end > > > up > > > to deference a NULL pointer. This can be prevented by moving the call > > > after we check the validity for the domain pointer. > > > > > > Coverity-ID: 1486741 > > > > The correct CID is 1486742 > > Hmmm yes. The coverity report e-mail is a bit confusing to read. > > However, I have already committed the patch so we will have to leave with it :(. > I guess the solution could be to fix another one and make a proper commit comment with cross-reference :) > Cheers, >
diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c index 3b30827764..cf7f25cda2 100644 --- a/xen/xsm/flask/hooks.c +++ b/xen/xsm/flask/hooks.c @@ -1296,11 +1296,13 @@ static int flask_assign_device(struct domain *d, uint32_t machine_bdf) u32 dsid, rsid; int rc = -EPERM; struct avc_audit_data ad; - u32 dperm = flask_iommu_resource_use_perm(d); + u32 dperm; if ( !d ) return flask_test_assign_device(machine_bdf); + dperm = flask_iommu_resource_use_perm(d); + rc = current_has_perm(d, SECCLASS_RESOURCE, RESOURCE__ADD); if ( rc ) return rc; @@ -1355,11 +1357,13 @@ static int flask_assign_dtdevice(struct domain *d, const char *dtpath) u32 dsid, rsid; int rc = -EPERM; struct avc_audit_data ad; - u32 dperm = flask_iommu_resource_use_perm(d); + u32 dperm; if ( !d ) return flask_test_assign_dtdevice(dtpath); + dperm = flask_iommu_resource_use_perm(d); + rc = current_has_perm(d, SECCLASS_RESOURCE, RESOURCE__ADD); if ( rc ) return rc;
flask_assign_{, dt}device() may be used to check whether you can test if a device is assigned. In this case, the domain will be NULL. However, flask_iommu_resource_use_perm() will be called and may end up to deference a NULL pointer. This can be prevented by moving the call after we check the validity for the domain pointer. Coverity-ID: 1486741 Fixes: 71e617a6b8 ('use is_iommu_enabled() where appropriate...') Signed-off-by: Julien Grall <julien.grall@arm.com> --- xen/xsm/flask/hooks.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)