[Qemu-devel] [PATCH] ide: ahci: add check to avoid null dereference (CVE-2019-12067)

[Qemu-devel] [PATCH] ide: ahci: add check to avoid null dereference (CVE-2019-12067)

[P J P]

From: Prasad J Pandit <pjp@fedoraproject.org>

AHCI emulator while committing DMA buffer in ahci_commit_buf()
may do a NULL dereference if the command header 'ad->cur_cmd'
is null. Add check to avoid it.

Reported-by: Bugs SysSec <bugs-syssec@rub.de>
Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
---
 hw/ide/ahci.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 00ba422a48..9fff94075b 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -1458,8 +1458,10 @@ static void ahci_commit_buf(IDEDMA *dma, uint32_t tx_bytes)
 {
     AHCIDevice *ad = DO_UPCAST(AHCIDevice, dma, dma);
 
-    tx_bytes += le32_to_cpu(ad->cur_cmd->status);
-    ad->cur_cmd->status = cpu_to_le32(tx_bytes);
+    if (ad->cur_cmd) {
+        tx_bytes += le32_to_cpu(ad->cur_cmd->status);
+        ad->cur_cmd->status = cpu_to_le32(tx_bytes);
+    }
 }
 
 static int ahci_dma_rw_buf(IDEDMA *dma, int is_write)
-- 
2.21.0

Re: [Qemu-devel] [PATCH] ide: ahci: add check to avoid null dereference (CVE-2019-12067)

[Philippe Mathieu-Daudé]

Hi Prasad,

On 8/8/19 8:56 AM, P J P wrote:
> From: Prasad J Pandit <pjp@fedoraproject.org>
> 
> AHCI emulator while committing DMA buffer in ahci_commit_buf()
> may do a NULL dereference if the command header 'ad->cur_cmd'
> is null. Add check to avoid it.
> 
> Reported-by: Bugs SysSec <bugs-syssec@rub.de>
> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
> ---
>  hw/ide/ahci.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
> index 00ba422a48..9fff94075b 100644
> --- a/hw/ide/ahci.c
> +++ b/hw/ide/ahci.c
> @@ -1458,8 +1458,10 @@ static void ahci_commit_buf(IDEDMA *dma, uint32_t tx_bytes)
>  {
>      AHCIDevice *ad = DO_UPCAST(AHCIDevice, dma, dma);
>  
> -    tx_bytes += le32_to_cpu(ad->cur_cmd->status);
> -    ad->cur_cmd->status = cpu_to_le32(tx_bytes);
> +    if (ad->cur_cmd) {

My 2 cents, John will correct me:

This is not a valid condition, so an assert() might be more appropriate
here.

Why is dma_buf_commit() being called with a null command? This check
should go elsewhere to avoid the call.

> +        tx_bytes += le32_to_cpu(ad->cur_cmd->status);
> +        ad->cur_cmd->status = cpu_to_le32(tx_bytes);
> +    }
>  }
>  
>  static int ahci_dma_rw_buf(IDEDMA *dma, int is_write)
> 

Re: [Qemu-devel] [PATCH] ide: ahci: add check to avoid null dereference (CVE-2019-12067)

[John Snow]

On 8/8/19 5:11 AM, Philippe Mathieu-Daudé wrote:
> Hi Prasad,
> 
> On 8/8/19 8:56 AM, P J P wrote:
>> From: Prasad J Pandit <pjp@fedoraproject.org>
>>
>> AHCI emulator while committing DMA buffer in ahci_commit_buf()
>> may do a NULL dereference if the command header 'ad->cur_cmd'
>> is null. Add check to avoid it.
>>
>> Reported-by: Bugs SysSec <bugs-syssec@rub.de>
>> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
>> ---
>>  hw/ide/ahci.c | 6 ++++--
>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
>> index 00ba422a48..9fff94075b 100644
>> --- a/hw/ide/ahci.c
>> +++ b/hw/ide/ahci.c
>> @@ -1458,8 +1458,10 @@ static void ahci_commit_buf(IDEDMA *dma, uint32_t tx_bytes)
>>  {
>>      AHCIDevice *ad = DO_UPCAST(AHCIDevice, dma, dma);
>>  
>> -    tx_bytes += le32_to_cpu(ad->cur_cmd->status);
>> -    ad->cur_cmd->status = cpu_to_le32(tx_bytes);
>> +    if (ad->cur_cmd) {
> 
> My 2 cents, John will correct me:
> 
> This is not a valid condition, so an assert() might be more appropriate
> here.
> 
> Why is dma_buf_commit() being called with a null command? This check
> should go elsewhere to avoid the call.
> 

Yes, something else is broken.

Can you please give a reproducer or more detailed report so I can fix
this properly?

This likely is just adding a whole host of new undefined problems
waiting to happen by adding an if (...), so while it's not nice to have
a host assert() it's probably better than taking the state machine off
the rails and seeing what else breaks.

>> +        tx_bytes += le32_to_cpu(ad->cur_cmd->status);
>> +        ad->cur_cmd->status = cpu_to_le32(tx_bytes);
>> +    }
>>  }
>>  
>>  static int ahci_dma_rw_buf(IDEDMA *dma, int is_write)
>>