Re: [Qemu-devel] [PATCH v1] tpm: check localities index

[P J P <ppandit@redhat.com>]

  Hello Stefan, Marc,

+-- On Tue, 20 Nov 2018, P J P wrote --+
| | On 11/20/18 2:22 AM, P J P wrote:
| | > From: Prasad J Pandit <pjp@fedoraproject.org>
| | > While performing mmio device r/w operations, guest could set 'addr'
| | > parameter such that 'locty' index exceeds TPM_TIS_NUM_LOCALITIES=5
| | > after setting new 'locty' via 'tpm_tis_new_active_locality'.
| | > Add check to avoid OOB access.
| | 
| | Do you have test code that can set the memory to such a locality or is this
| | purely hypothetical at the moment?
| 
| It's not hypothetical, reporter used tpm-emulator below
| 
|   -> https://github.com/PeterHuewe/tpm-emulator
| 
| with an old version of QEMU. I shared the details with Marc-Andre earlier 
| today.
| 
| | We are registering this MMIO area:
| | 
| |     memory_region_init_io(&s->mmio, OBJECT(s), &tpm_tis_memory_ops,
| |                           s, "tpm-tis-mmio",
| |                           TPM_TIS_NUM_LOCALITIES << 
| | TPM_TIS_LOCALITY_SHIFT);
| | 
| | #define TPM_TIS_NUM_LOCALITIES      5
| | 
| | #define TPM_TIS_LOCALITY_SHIFT      12
| | 
| | --> 5 << 12 = 0x5000, thus we get memory locations [0 .. 0x4fff]
| | 
| | 
| | We have the following code to get from an address to the locality:
| | 
| | static uint8_t tpm_tis_locality_from_addr(hwaddr addr)
| | {
| |     return (uint8_t)((addr >> TPM_TIS_LOCALITY_SHIFT) & 0x7);
| | }
| | 
| | With this we would get localities of [0x0 .. 0x4] following the memory
| | locations [0 .. 0x4fff] above.
| 
|   Right. IIUC mmio r/w routines called with 'addr' value like 0x07000 may set 
| 'locty' to > TPM_TIS_NUM_LOCALITIES. Also in mmio_write at one point, 
| 'active_locty' is set to TPM_TIS_NO_LOCALITY(=0xff), which may be used to set 
| 'new_locty'.
| 
| tpm_tis_mmio_write
|    ...
|    active_locty = TPM_TIS_NO_LOCALITY(=0xff);
|    ...
|    tpm_tis_new_active_locality(s, active_locty);
| 
| 
| tpm_tis_new_active_locality(TPMState *s, uint8_t new_active_locty)
|    ...
|    s->active_locty = new_active_locty;
| 

To confirm,
  is the proposed patch v1 good/ack'ed for upstream?

Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F