Re: [Qemu-devel] [PATCH] hmp: avoid redundant null termination of buffer

[Markus Armbruster]

Luiz Capitulino <address@hidden> writes:

> On Thu, 17 Dec 2015 18:10:59 +0530 (IST)
> P J P <address@hidden> wrote:
>
>>    Hello,
>> 
>> An OOB write issue was reported by Mr Ling Liu, CC'd here. It occurs while 
>> processing the 'sendkey' command, if the command argument was longer than
>> the 'keyname_buf[16]' buffer.
>
> Markus, are you planning a pull request soon?

I have nothing queued for the monitor, but I can take this fix or the
proposed alternative through my tree anyway.

> Reviewed-by: Luiz Capitulino <address@hidden>

>> 
>> ===
>> From b0363f4c0e91671064dd7ffece8a6923c8dcaf20 Mon Sep 17 00:00:00 2001
>> From: Prasad J Pandit <address@hidden>
>> Date: Thu, 17 Dec 2015 17:47:15 +0530
>> Subject: [PATCH] hmp: avoid redundant null termination of buffer
>> 
>> When processing 'sendkey' command, hmp_sendkey routine null
>> terminates the 'keyname_buf' array. This results in an OOB write
>> issue, if 'keyname_len' was to fall outside of 'keyname_buf' array.
>> Removed the redundant null termination, as pstrcpy routine already
>> null terminates the target buffer.
>> 
>> Reported-by: Ling Liu <address@hidden>
>> Signed-off-by: Prasad J Pandit <address@hidden>
>> ---
>>   hmp.c | 2 --
>>   1 file changed, 2 deletions(-)
>> 
>> diff --git a/hmp.c b/hmp.c
>> index 2140605..e530c9c 100644
>> --- a/hmp.c
>> +++ b/hmp.c
>> @@ -1746,9 +1746,7 @@ void hmp_sendkey(Monitor *mon, const QDict *qdict)
>>           /* Be compatible with old interface, convert user inputted "<" */
>>           if (!strncmp(keyname_buf, "<", 1) && keyname_len == 1) {
>>               pstrcpy(keyname_buf, sizeof(keyname_buf), "less");
>> -            keyname_len = 4;
>>           }
>> -         keyname_buf[keyname_len] = 0;
>> 
>>           keylist = g_malloc0(sizeof(*keylist));
>>           keylist->value = g_malloc0(sizeof(*keylist->value));

Minimally invasive fix, works for me.

Reviewed-by: Markus Armbruster <address@hidden>