hv: vtd: fix potential buffer overflow in suspend/resume #1253
Conversation
| pr_err("iommu dev number is larger than pre-defined"); | ||
| break; | ||
| } | ||
| iommu_idx++; |
There was a problem hiding this comment.
should we remove definition of iommu_idx?
hypervisor/arch/x86/vtd.c
Outdated
| @@ -30,6 +30,10 @@ | |||
| #define ROOT_ENTRY_LOWER_CTP_POS (12U) | |||
| #define ROOT_ENTRY_LOWER_CTP_MASK (0xFFFFFFFFFFFFFUL) | |||
|
|
|||
| /* 4 iommu fault register state */ | |||
| #define IOMMU_FAULT_REGISTER_STATE_NUM 4U | |||
| #define IOMMU_FAULT_REGISTER_STATE_SIZE 4U | |||
There was a problem hiding this comment.
This MACRO is actually sizeof(failt_state), can we do in this way?
It helps to couple them together.
There was a problem hiding this comment.
It is the register width/register address offset of these fault registers. And it is fixed according to vtd spec. sizeof(fault_state) kind of related the hardware register offset to the data structure we defined, which I not prefer to. If we want to get rid of a new macro, I would prefer to use sizeof (uint32_t).
binbinwu1
force-pushed
the
vtd_suspend_resume
branch
from
52f4c6b
to
7800a99
Compare
hypervisor/arch/x86/vtd.c
Outdated
| iommu_read32(dmar_unit, DMAR_FECTL_REG + | ||
| (i * IOMMU_FAULT_REGISTER_STATE_NUM)); | ||
| dmar_unit->fault_state[i] = iommu_read32(dmar_unit, | ||
| DMAR_FECTL_REG + (i * sizeof(uint32_t))); |
There was a problem hiding this comment.
sizeof(uint32_t) is not good --- if the definition side changes, it still lead to bug. should be sizeof(fault_state).
for example sizeof( ((struct dmar_drhd_rt*) NULL) ->fault_state[0]).
There was a problem hiding this comment.
This value is not related to definition side of fault_state, it should be aligned with hardware spec as below, so it should be a constant value:
#define DMAR_FSTS_REG 0x34U /* Fault Status register /
#define DMAR_FECTL_REG 0x38U / Fault control register /
#define DMAR_FEDATA_REG 0x3cU / Fault event interrupt data register /
#define DMAR_FEADDR_REG 0x40U / Fault event interrupt addr register */
There was a problem hiding this comment.
Spec is the source of the requirement. data structure definition is following the spec.
The MACRO could derive from the data structure or from the original spec. Either way is better than sizeof(uint32_t) here, which follows neither of the source.
binbinwu1
force-pushed
the
vtd_suspend_resume
branch
from
7800a99
to
b9518d6
Compare
In current code of suspend_iommu/resume_iommu, there is potential buffer overflow according to the code. This patch put the buffer to struct dmar_drhd_rt, so that no need to access the buffer via index. Signed-off-by: Binbin Wu <binbin.wu@intel.com> Tracked-On: projectacrn#1252 Acked-by: Eddie Dong <eddie.dong@intel.com>
binbinwu1
force-pushed
the
vtd_suspend_resume
branch
from
b9518d6
to
d674736
Compare
|
Ready to merge |
In current code of suspend_iommu/resume_iommu, there is potential buffer overflow
according to the code.
This patch put the buffer to struct dmar_drhd_rt, so that no need to access the buffer
via index.
Signed-off-by: Binbin Wu binbin.wu@intel.com
Tracked-On: #1252
Acked-by: Eddie Dong eddie.dong@intel.com