[Qemu-devel] [PATCH] fdc: force the fifo access to be in bounds of the allocated buffer

[Qemu-devel] [PATCH] fdc: force the fifo access to be in bounds of the allocated buffer

[John Snow]

From: Petr Matousek <pmatouse@redhat.com>

During processing of certain commands such as FD_CMD_READ_ID and
FD_CMD_DRIVE_SPECIFICATION_COMMAND the fifo memory access could
get out of bounds leading to memory corruption with values coming
from the guest.

Fix this by making sure that the index is always bounded by the
allocated memory.

This is CVE-2015-3456.

Signed-off-by: Petr Matousek <pmatouse@redhat.com>
Reviewed-by: John Snow <jsnow@redhat.com>
Signed-off-by: John Snow <jsnow@redhat.com>
---
 hw/block/fdc.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index f72a392..d8a8edd 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -1497,7 +1497,7 @@ static uint32_t fdctrl_read_data(FDCtrl *fdctrl)
 {
     FDrive *cur_drv;
     uint32_t retval = 0;
-    int pos;
+    uint32_t pos;
 
     cur_drv = get_cur_drv(fdctrl);
     fdctrl->dsr &= ~FD_DSR_PWRDOWN;
@@ -1506,8 +1506,8 @@ static uint32_t fdctrl_read_data(FDCtrl *fdctrl)
         return 0;
     }
     pos = fdctrl->data_pos;
+    pos %= FD_SECTOR_LEN;
     if (fdctrl->msr & FD_MSR_NONDMA) {
-        pos %= FD_SECTOR_LEN;
         if (pos == 0) {
             if (fdctrl->data_pos != 0)
                 if (!fdctrl_seek_to_next_sect(fdctrl, cur_drv)) {
@@ -1852,10 +1852,13 @@ static void fdctrl_handle_option(FDCtrl *fdctrl, int direction)
 static void fdctrl_handle_drive_specification_command(FDCtrl *fdctrl, int direction)
 {
     FDrive *cur_drv = get_cur_drv(fdctrl);
+    uint32_t pos;
 
-    if (fdctrl->fifo[fdctrl->data_pos - 1] & 0x80) {
+    pos = fdctrl->data_pos - 1;
+    pos %= FD_SECTOR_LEN;
+    if (fdctrl->fifo[pos] & 0x80) {
         /* Command parameters done */
-        if (fdctrl->fifo[fdctrl->data_pos - 1] & 0x40) {
+        if (fdctrl->fifo[pos] & 0x40) {
             fdctrl->fifo[0] = fdctrl->fifo[1];
             fdctrl->fifo[2] = 0;
             fdctrl->fifo[3] = 0;
@@ -1955,7 +1958,7 @@ static uint8_t command_to_handler[256];
 static void fdctrl_write_data(FDCtrl *fdctrl, uint32_t value)
 {
     FDrive *cur_drv;
-    int pos;
+    uint32_t pos;
 
     /* Reset mode */
     if (!(fdctrl->dor & FD_DOR_nRESET)) {
@@ -2004,7 +2007,9 @@ static void fdctrl_write_data(FDCtrl *fdctrl, uint32_t value)
     }
 
     FLOPPY_DPRINTF("%s: %02x\n", __func__, value);
-    fdctrl->fifo[fdctrl->data_pos++] = value;
+    pos = fdctrl->data_pos++;
+    pos %= FD_SECTOR_LEN;
+    fdctrl->fifo[pos] = value;
     if (fdctrl->data_pos == fdctrl->data_len) {
         /* We now have all parameters
          * and will be able to treat the command
-- 
2.1.0

[Qemu-devel] [PATCH] fdc: force the fifo access to be in bounds of the allocated buffer

[John Snow]

From: Petr Matousek <pmatouse@redhat.com>

During processing of certain commands such as FD_CMD_READ_ID and
FD_CMD_DRIVE_SPECIFICATION_COMMAND the fifo memory access could
get out of bounds leading to memory corruption with values coming
from the guest.

Fix this by making sure that the index is always bounded by the
allocated memory.

This is CVE-2015-3456.

Signed-off-by: Petr Matousek <pmatouse@redhat.com>
Reviewed-by: John Snow <jsnow@redhat.com>
Signed-off-by: John Snow <jsnow@redhat.com>
---
 hw/block/fdc.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index f72a392..d8a8edd 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -1497,7 +1497,7 @@ static uint32_t fdctrl_read_data(FDCtrl *fdctrl)
 {
     FDrive *cur_drv;
     uint32_t retval = 0;
-    int pos;
+    uint32_t pos;
 
     cur_drv = get_cur_drv(fdctrl);
     fdctrl->dsr &= ~FD_DSR_PWRDOWN;
@@ -1506,8 +1506,8 @@ static uint32_t fdctrl_read_data(FDCtrl *fdctrl)
         return 0;
     }
     pos = fdctrl->data_pos;
+    pos %= FD_SECTOR_LEN;
     if (fdctrl->msr & FD_MSR_NONDMA) {
-        pos %= FD_SECTOR_LEN;
         if (pos == 0) {
             if (fdctrl->data_pos != 0)
                 if (!fdctrl_seek_to_next_sect(fdctrl, cur_drv)) {
@@ -1852,10 +1852,13 @@ static void fdctrl_handle_option(FDCtrl *fdctrl, int direction)
 static void fdctrl_handle_drive_specification_command(FDCtrl *fdctrl, int direction)
 {
     FDrive *cur_drv = get_cur_drv(fdctrl);
+    uint32_t pos;
 
-    if (fdctrl->fifo[fdctrl->data_pos - 1] & 0x80) {
+    pos = fdctrl->data_pos - 1;
+    pos %= FD_SECTOR_LEN;
+    if (fdctrl->fifo[pos] & 0x80) {
         /* Command parameters done */
-        if (fdctrl->fifo[fdctrl->data_pos - 1] & 0x40) {
+        if (fdctrl->fifo[pos] & 0x40) {
             fdctrl->fifo[0] = fdctrl->fifo[1];
             fdctrl->fifo[2] = 0;
             fdctrl->fifo[3] = 0;
@@ -1955,7 +1958,7 @@ static uint8_t command_to_handler[256];
 static void fdctrl_write_data(FDCtrl *fdctrl, uint32_t value)
 {
     FDrive *cur_drv;
-    int pos;
+    uint32_t pos;
 
     /* Reset mode */
     if (!(fdctrl->dor & FD_DOR_nRESET)) {
@@ -2004,7 +2007,9 @@ static void fdctrl_write_data(FDCtrl *fdctrl, uint32_t value)
     }
 
     FLOPPY_DPRINTF("%s: %02x\n", __func__, value);
-    fdctrl->fifo[fdctrl->data_pos++] = value;
+    pos = fdctrl->data_pos++;
+    pos %= FD_SECTOR_LEN;
+    fdctrl->fifo[pos] = value;
     if (fdctrl->data_pos == fdctrl->data_len) {
         /* We now have all parameters
          * and will be able to treat the command
-- 
2.1.0

Re: [Qemu-devel] [PATCH] fdc: force the fifo access to be in bounds of the allocated buffer

[John Snow]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256



On 05/13/2015 10:33 AM, John Snow wrote:
> From: Petr Matousek <pmatouse@redhat.com>
> 
> During processing of certain commands such as FD_CMD_READ_ID and 
> FD_CMD_DRIVE_SPECIFICATION_COMMAND the fifo memory access could get
> out of bounds leading to memory corruption with values coming from
> the guest.
> 
> Fix this by making sure that the index is always bounded by the 
> allocated memory.
> 
> This is CVE-2015-3456.
> 
> Signed-off-by: Petr Matousek <pmatouse@redhat.com> Reviewed-by:
> John Snow <jsnow@redhat.com> Signed-off-by: John Snow
> <jsnow@redhat.com> ---
[snip]

Already sent the pull request (at 08:00 EDT this morning) for
inclusion in the master branch, but this will serve as the formal
patch discussion / and request for inclusion into any stable branches
still being maintained.

Thanks.

- --John Snow
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQIcBAEBCAAGBQJVU2EtAAoJEH3vgQaq/DkO+ogP/1D1W2F4hbqV+CDakrCLJagz
wC/XiGmixY+CUHr8z+OjXLtJLkSj2HprdbY3S1ogeJUOLXHUePYGBBEwjjH/Ed7b
TPYjzfEZlmw5UzMIGOIIZfHtOA5Xzsq0Ipqk5PXOXyprm0aDji9ZMwRTkdTbwuYI
kBps6ajkHNkzxIIRO11aWJjiRo0CfIEFZgLrYRVdtixzfgeEHJRfGJJvOA3VIrwD
5yS2tjgpkrj4C4tO/gdOeOUfmiwh5IjSHPVwgEkTABZxe4FFxEs9oGuReKyZFcq9
/60nqJ689+JxMxoPtPcQDvwf9tSmOWG1RRe3m+NwhY3lLuIhmpIDnjABSvFJhUye
v9gd52jf/mOO557iUh/I8JbdZLc8NPcR8C9JC1zGewYFk7lKEsVUUaAyw1QkrrVa
7GfpjjnXeys8HkBgNNmjtLnq6V15rFA5B8Oc0yyhSRXZimIIkF6C+G8pnv8GdonL
n7Sm1nsFnhVeinK37dSDMHBqKqRKGyJE6HRGniP9xMluycxf9mtNMKpBmPmmTHPd
QjjScqrWQTJd12Hlzsh7HnoNNBQ/nG6Om45/PKsoVWaByc7d7XQ0yw3BI3xLxQMb
yzmstCgAg5K+pbt2MJsPBMJCCOuda2scCSWAWVFAX306sdcV5ZUhr6wpnhlCV1lI
UEjPHAmhLUUqrZDQHuH0
=gUNa
-----END PGP SIGNATURE-----

Re: [Qemu-devel] [PATCH] fdc: force the fifo access to be in bounds of the allocated buffer

[Stefan Weil]

Hi,

I just noticed this patch because my provider told me that my KVM based 
server
needs a reboot because of a CVE (see this German news: 
http://www.heise.de/newsticker/meldung/Venom-Schwachstelle-Aus-Hypervisor-ausbrechen-und-VMs-ausspionieren-2649614.html)

Am 13.05.2015 um 16:33 schrieb John Snow:
> From: Petr Matousek <pmatouse@redhat.com>
>
> During processing of certain commands such as FD_CMD_READ_ID and
> FD_CMD_DRIVE_SPECIFICATION_COMMAND the fifo memory access could
> get out of bounds leading to memory corruption with values coming
> from the guest.
>
> Fix this by making sure that the index is always bounded by the
> allocated memory.
>
> This is CVE-2015-3456.
>
> Signed-off-by: Petr Matousek <pmatouse@redhat.com>
> Reviewed-by: John Snow <jsnow@redhat.com>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   hw/block/fdc.c | 17 +++++++++++------
>   1 file changed, 11 insertions(+), 6 deletions(-)
>
> diff --git a/hw/block/fdc.c b/hw/block/fdc.c
> index f72a392..d8a8edd 100644
> --- a/hw/block/fdc.c
> +++ b/hw/block/fdc.c
> @@ -1497,7 +1497,7 @@ static uint32_t fdctrl_read_data(FDCtrl *fdctrl)
>   {
>       FDrive *cur_drv;
>       uint32_t retval = 0;
> -    int pos;
> +    uint32_t pos;
>   
>       cur_drv = get_cur_drv(fdctrl);
>       fdctrl->dsr &= ~FD_DSR_PWRDOWN;
> @@ -1506,8 +1506,8 @@ static uint32_t fdctrl_read_data(FDCtrl *fdctrl)
>           return 0;
>       }
>       pos = fdctrl->data_pos;
> +    pos %= FD_SECTOR_LEN;

I'd combine both statements and perhaps use fdctrl->fifo_size (even if 
the resulting code will be slightly larger):

pos = fdctrl->data_pos % fdctrl->fifo_size;


>       if (fdctrl->msr & FD_MSR_NONDMA) {
> -        pos %= FD_SECTOR_LEN;
>           if (pos == 0) {
>               if (fdctrl->data_pos != 0)
>                   if (!fdctrl_seek_to_next_sect(fdctrl, cur_drv)) {
> @@ -1852,10 +1852,13 @@ static void fdctrl_handle_option(FDCtrl *fdctrl, int direction)
>   static void fdctrl_handle_drive_specification_command(FDCtrl *fdctrl, int direction)
>   {
>       FDrive *cur_drv = get_cur_drv(fdctrl);
> +    uint32_t pos;
>   
> -    if (fdctrl->fifo[fdctrl->data_pos - 1] & 0x80) {
> +    pos = fdctrl->data_pos - 1;
> +    pos %= FD_SECTOR_LEN;

Shorter (and more clear):

uint32_t pos = (fdctrl->data_pos - 1) % fdctrl->fifo_size;

> +    if (fdctrl->fifo[pos] & 0x80) {
>           /* Command parameters done */
> -        if (fdctrl->fifo[fdctrl->data_pos - 1] & 0x40) {
> +        if (fdctrl->fifo[pos] & 0x40) {
>               fdctrl->fifo[0] = fdctrl->fifo[1];
>               fdctrl->fifo[2] = 0;
>               fdctrl->fifo[3] = 0;
> @@ -1955,7 +1958,7 @@ static uint8_t command_to_handler[256];
>   static void fdctrl_write_data(FDCtrl *fdctrl, uint32_t value)
>   {
>       FDrive *cur_drv;
> -    int pos;
> +    uint32_t pos;
>   
>       /* Reset mode */
>       if (!(fdctrl->dor & FD_DOR_nRESET)) {
> @@ -2004,7 +2007,9 @@ static void fdctrl_write_data(FDCtrl *fdctrl, uint32_t value)
>       }
>   
>       FLOPPY_DPRINTF("%s: %02x\n", __func__, value);
> -    fdctrl->fifo[fdctrl->data_pos++] = value;
> +    pos = fdctrl->data_pos++;
> +    pos %= FD_SECTOR_LEN;
> +    fdctrl->fifo[pos] = value;
>       if (fdctrl->data_pos == fdctrl->data_len) {
>           /* We now have all parameters
>            * and will be able to treat the command

Not strictly related to this patch: The code which sets fifo_size could 
also be improved.

     fdctrl->fifo = qemu_memalign(512, FD_SECTOR_LEN);
     fdctrl->fifo_size = 512;

The 2nd line should be

     fdctrl->fifo_size = FD_SECTOR_LEN;


As far as I see the original code can read or write illegal memory 
locations in the address space of the QEMU process. It cannot (as it was 
claimed) modify the code of the VM host because those memory is usually 
write protected - at least if QEMU is running without KVM. If the code 
which is generated for KVM is writable from anywhere in QEMU, we should 
perhaps consider changing that.

Regards
Stefan

Re: [Qemu-devel] [Qemu-stable] [PATCH] fdc: force the fifo access to be in bounds of the allocated buffer

[Stefan Priebe]

Am 13.05.2015 um 20:51 schrieb Stefan Weil:
> Hi,
>
> I just noticed this patch because my provider told me that my KVM based
> server
> needs a reboot because of a CVE (see this German news:
> http://www.heise.de/newsticker/meldung/Venom-Schwachstelle-Aus-Hypervisor-ausbrechen-und-VMs-ausspionieren-2649614.html)

Isn't a live migration to a fixed version enough instead of a reboot?

Stefan


>
> Am 13.05.2015 um 16:33 schrieb John Snow:
>> From: Petr Matousek <pmatouse@redhat.com>
>>
>> During processing of certain commands such as FD_CMD_READ_ID and
>> FD_CMD_DRIVE_SPECIFICATION_COMMAND the fifo memory access could
>> get out of bounds leading to memory corruption with values coming
>> from the guest.
>>
>> Fix this by making sure that the index is always bounded by the
>> allocated memory.
>>
>> This is CVE-2015-3456.
>>
>> Signed-off-by: Petr Matousek <pmatouse@redhat.com>
>> Reviewed-by: John Snow <jsnow@redhat.com>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>   hw/block/fdc.c | 17 +++++++++++------
>>   1 file changed, 11 insertions(+), 6 deletions(-)
>>
>> diff --git a/hw/block/fdc.c b/hw/block/fdc.c
>> index f72a392..d8a8edd 100644
>> --- a/hw/block/fdc.c
>> +++ b/hw/block/fdc.c
>> @@ -1497,7 +1497,7 @@ static uint32_t fdctrl_read_data(FDCtrl *fdctrl)
>>   {
>>       FDrive *cur_drv;
>>       uint32_t retval = 0;
>> -    int pos;
>> +    uint32_t pos;
>>       cur_drv = get_cur_drv(fdctrl);
>>       fdctrl->dsr &= ~FD_DSR_PWRDOWN;
>> @@ -1506,8 +1506,8 @@ static uint32_t fdctrl_read_data(FDCtrl *fdctrl)
>>           return 0;
>>       }
>>       pos = fdctrl->data_pos;
>> +    pos %= FD_SECTOR_LEN;
>
> I'd combine both statements and perhaps use fdctrl->fifo_size (even if
> the resulting code will be slightly larger):
>
> pos = fdctrl->data_pos % fdctrl->fifo_size;
>
>
>>       if (fdctrl->msr & FD_MSR_NONDMA) {
>> -        pos %= FD_SECTOR_LEN;
>>           if (pos == 0) {
>>               if (fdctrl->data_pos != 0)
>>                   if (!fdctrl_seek_to_next_sect(fdctrl, cur_drv)) {
>> @@ -1852,10 +1852,13 @@ static void fdctrl_handle_option(FDCtrl
>> *fdctrl, int direction)
>>   static void fdctrl_handle_drive_specification_command(FDCtrl
>> *fdctrl, int direction)
>>   {
>>       FDrive *cur_drv = get_cur_drv(fdctrl);
>> +    uint32_t pos;
>> -    if (fdctrl->fifo[fdctrl->data_pos - 1] & 0x80) {
>> +    pos = fdctrl->data_pos - 1;
>> +    pos %= FD_SECTOR_LEN;
>
> Shorter (and more clear):
>
> uint32_t pos = (fdctrl->data_pos - 1) % fdctrl->fifo_size;
>
>> +    if (fdctrl->fifo[pos] & 0x80) {
>>           /* Command parameters done */
>> -        if (fdctrl->fifo[fdctrl->data_pos - 1] & 0x40) {
>> +        if (fdctrl->fifo[pos] & 0x40) {
>>               fdctrl->fifo[0] = fdctrl->fifo[1];
>>               fdctrl->fifo[2] = 0;
>>               fdctrl->fifo[3] = 0;
>> @@ -1955,7 +1958,7 @@ static uint8_t command_to_handler[256];
>>   static void fdctrl_write_data(FDCtrl *fdctrl, uint32_t value)
>>   {
>>       FDrive *cur_drv;
>> -    int pos;
>> +    uint32_t pos;
>>       /* Reset mode */
>>       if (!(fdctrl->dor & FD_DOR_nRESET)) {
>> @@ -2004,7 +2007,9 @@ static void fdctrl_write_data(FDCtrl *fdctrl,
>> uint32_t value)
>>       }
>>       FLOPPY_DPRINTF("%s: %02x\n", __func__, value);
>> -    fdctrl->fifo[fdctrl->data_pos++] = value;
>> +    pos = fdctrl->data_pos++;
>> +    pos %= FD_SECTOR_LEN;
>> +    fdctrl->fifo[pos] = value;
>>       if (fdctrl->data_pos == fdctrl->data_len) {
>>           /* We now have all parameters
>>            * and will be able to treat the command
>
> Not strictly related to this patch: The code which sets fifo_size could
> also be improved.
>
>      fdctrl->fifo = qemu_memalign(512, FD_SECTOR_LEN);
>      fdctrl->fifo_size = 512;
>
> The 2nd line should be
>
>      fdctrl->fifo_size = FD_SECTOR_LEN;
>
>
> As far as I see the original code can read or write illegal memory
> locations in the address space of the QEMU process. It cannot (as it was
> claimed) modify the code of the VM host because those memory is usually
> write protected - at least if QEMU is running without KVM. If the code
> which is generated for KVM is writable from anywhere in QEMU, we should
> perhaps consider changing that.
>
> Regards
> Stefan
>
>

Re: [Qemu-devel] [Qemu-stable] [PATCH] fdc: force the fifo access to be in bounds of the allocated buffer

[John Snow]

On 05/13/2015 02:59 PM, Stefan Priebe wrote:
> 
> Am 13.05.2015 um 20:51 schrieb Stefan Weil:
>> Hi,
>>
>> I just noticed this patch because my provider told me that my KVM based
>> server
>> needs a reboot because of a CVE (see this German news:
>> http://www.heise.de/newsticker/meldung/Venom-Schwachstelle-Aus-Hypervisor-ausbrechen-und-VMs-ausspionieren-2649614.html)
>>
> 
> Isn't a live migration to a fixed version enough instead of a reboot?
> 
> Stefan
> 
> 

If your management API or host or whatever lets you migrate back to the
same host, or has another host they can migrate you to, yes.

>>
>> Am 13.05.2015 um 16:33 schrieb John Snow:
>>> From: Petr Matousek <pmatouse@redhat.com>
>>>
>>> During processing of certain commands such as FD_CMD_READ_ID and
>>> FD_CMD_DRIVE_SPECIFICATION_COMMAND the fifo memory access could
>>> get out of bounds leading to memory corruption with values coming
>>> from the guest.
>>>
>>> Fix this by making sure that the index is always bounded by the
>>> allocated memory.
>>>
>>> This is CVE-2015-3456.
>>>
>>> Signed-off-by: Petr Matousek <pmatouse@redhat.com>
>>> Reviewed-by: John Snow <jsnow@redhat.com>
>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>> ---
>>>   hw/block/fdc.c | 17 +++++++++++------
>>>   1 file changed, 11 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/hw/block/fdc.c b/hw/block/fdc.c
>>> index f72a392..d8a8edd 100644
>>> --- a/hw/block/fdc.c
>>> +++ b/hw/block/fdc.c
>>> @@ -1497,7 +1497,7 @@ static uint32_t fdctrl_read_data(FDCtrl *fdctrl)
>>>   {
>>>       FDrive *cur_drv;
>>>       uint32_t retval = 0;
>>> -    int pos;
>>> +    uint32_t pos;
>>>       cur_drv = get_cur_drv(fdctrl);
>>>       fdctrl->dsr &= ~FD_DSR_PWRDOWN;
>>> @@ -1506,8 +1506,8 @@ static uint32_t fdctrl_read_data(FDCtrl *fdctrl)
>>>           return 0;
>>>       }
>>>       pos = fdctrl->data_pos;
>>> +    pos %= FD_SECTOR_LEN;
>>
>> I'd combine both statements and perhaps use fdctrl->fifo_size (even if
>> the resulting code will be slightly larger):
>>
>> pos = fdctrl->data_pos % fdctrl->fifo_size;
>>
>>
>>>       if (fdctrl->msr & FD_MSR_NONDMA) {
>>> -        pos %= FD_SECTOR_LEN;
>>>           if (pos == 0) {
>>>               if (fdctrl->data_pos != 0)
>>>                   if (!fdctrl_seek_to_next_sect(fdctrl, cur_drv)) {
>>> @@ -1852,10 +1852,13 @@ static void fdctrl_handle_option(FDCtrl
>>> *fdctrl, int direction)
>>>   static void fdctrl_handle_drive_specification_command(FDCtrl
>>> *fdctrl, int direction)
>>>   {
>>>       FDrive *cur_drv = get_cur_drv(fdctrl);
>>> +    uint32_t pos;
>>> -    if (fdctrl->fifo[fdctrl->data_pos - 1] & 0x80) {
>>> +    pos = fdctrl->data_pos - 1;
>>> +    pos %= FD_SECTOR_LEN;
>>
>> Shorter (and more clear):
>>
>> uint32_t pos = (fdctrl->data_pos - 1) % fdctrl->fifo_size;
>>
>>> +    if (fdctrl->fifo[pos] & 0x80) {
>>>           /* Command parameters done */
>>> -        if (fdctrl->fifo[fdctrl->data_pos - 1] & 0x40) {
>>> +        if (fdctrl->fifo[pos] & 0x40) {
>>>               fdctrl->fifo[0] = fdctrl->fifo[1];
>>>               fdctrl->fifo[2] = 0;
>>>               fdctrl->fifo[3] = 0;
>>> @@ -1955,7 +1958,7 @@ static uint8_t command_to_handler[256];
>>>   static void fdctrl_write_data(FDCtrl *fdctrl, uint32_t value)
>>>   {
>>>       FDrive *cur_drv;
>>> -    int pos;
>>> +    uint32_t pos;
>>>       /* Reset mode */
>>>       if (!(fdctrl->dor & FD_DOR_nRESET)) {
>>> @@ -2004,7 +2007,9 @@ static void fdctrl_write_data(FDCtrl *fdctrl,
>>> uint32_t value)
>>>       }
>>>       FLOPPY_DPRINTF("%s: %02x\n", __func__, value);
>>> -    fdctrl->fifo[fdctrl->data_pos++] = value;
>>> +    pos = fdctrl->data_pos++;
>>> +    pos %= FD_SECTOR_LEN;
>>> +    fdctrl->fifo[pos] = value;
>>>       if (fdctrl->data_pos == fdctrl->data_len) {
>>>           /* We now have all parameters
>>>            * and will be able to treat the command
>>
>> Not strictly related to this patch: The code which sets fifo_size could
>> also be improved.
>>
>>      fdctrl->fifo = qemu_memalign(512, FD_SECTOR_LEN);
>>      fdctrl->fifo_size = 512;
>>
>> The 2nd line should be
>>
>>      fdctrl->fifo_size = FD_SECTOR_LEN;
>>
>>
>> As far as I see the original code can read or write illegal memory
>> locations in the address space of the QEMU process. It cannot (as it was
>> claimed) modify the code of the VM host because those memory is usually
>> write protected - at least if QEMU is running without KVM. If the code
>> which is generated for KVM is writable from anywhere in QEMU, we should
>> perhaps consider changing that.
>>
>> Regards
>> Stefan
>>
>>
> 

-- 
—js

Re: [Qemu-devel] [Qemu-stable] [PATCH] fdc: force the fifo access to be in bounds of the allocated buffer

[Stefan Weil]

Am 13.05.2015 um 20:59 schrieb Stefan Priebe:
>
> Am 13.05.2015 um 20:51 schrieb Stefan Weil:
>> Hi,
>>
>> I just noticed this patch because my provider told me that my KVM based
>> server
>> needs a reboot because of a CVE (see this German news:
>> http://www.heise.de/newsticker/meldung/Venom-Schwachstelle-Aus-Hypervisor-ausbrechen-und-VMs-ausspionieren-2649614.html) 
>>
>
> Isn't a live migration to a fixed version enough instead of a reboot?
>
> Stefan

Good point. A live migration would be sufficient - if there are no bugs
in QEMU's live migration.

Stefan

Re: [Qemu-devel] [Qemu-stable] [PATCH] fdc: force the fifo access to be in bounds of the allocated buffer

[Stefan Priebe]

Am 13.05.2015 um 21:04 schrieb John Snow:
>
>
> On 05/13/2015 02:59 PM, Stefan Priebe wrote:
>>
>> Am 13.05.2015 um 20:51 schrieb Stefan Weil:
>>> Hi,
>>>
>>> I just noticed this patch because my provider told me that my KVM based
>>> server
>>> needs a reboot because of a CVE (see this German news:
>>> http://www.heise.de/newsticker/meldung/Venom-Schwachstelle-Aus-Hypervisor-ausbrechen-und-VMs-ausspionieren-2649614.html)
>>>
>>
>> Isn't a live migration to a fixed version enough instead of a reboot?
>>
>> Stefan
>>
>>
>
> If your management API or host or whatever lets you migrate back to the
> same host, or has another host they can migrate you to, yes.

i'm my host ;-) I was just interested if for whatever reason live 
migration is not enough and was wondering why someone wants to reboot a 
vm for something like this.

Thanks!

Stefan

>
>>>
>>> Am 13.05.2015 um 16:33 schrieb John Snow:
>>>> From: Petr Matousek <pmatouse@redhat.com>
>>>>
>>>> During processing of certain commands such as FD_CMD_READ_ID and
>>>> FD_CMD_DRIVE_SPECIFICATION_COMMAND the fifo memory access could
>>>> get out of bounds leading to memory corruption with values coming
>>>> from the guest.
>>>>
>>>> Fix this by making sure that the index is always bounded by the
>>>> allocated memory.
>>>>
>>>> This is CVE-2015-3456.
>>>>
>>>> Signed-off-by: Petr Matousek <pmatouse@redhat.com>
>>>> Reviewed-by: John Snow <jsnow@redhat.com>
>>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>>> ---
>>>>    hw/block/fdc.c | 17 +++++++++++------
>>>>    1 file changed, 11 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/hw/block/fdc.c b/hw/block/fdc.c
>>>> index f72a392..d8a8edd 100644
>>>> --- a/hw/block/fdc.c
>>>> +++ b/hw/block/fdc.c
>>>> @@ -1497,7 +1497,7 @@ static uint32_t fdctrl_read_data(FDCtrl *fdctrl)
>>>>    {
>>>>        FDrive *cur_drv;
>>>>        uint32_t retval = 0;
>>>> -    int pos;
>>>> +    uint32_t pos;
>>>>        cur_drv = get_cur_drv(fdctrl);
>>>>        fdctrl->dsr &= ~FD_DSR_PWRDOWN;
>>>> @@ -1506,8 +1506,8 @@ static uint32_t fdctrl_read_data(FDCtrl *fdctrl)
>>>>            return 0;
>>>>        }
>>>>        pos = fdctrl->data_pos;
>>>> +    pos %= FD_SECTOR_LEN;
>>>
>>> I'd combine both statements and perhaps use fdctrl->fifo_size (even if
>>> the resulting code will be slightly larger):
>>>
>>> pos = fdctrl->data_pos % fdctrl->fifo_size;
>>>
>>>
>>>>        if (fdctrl->msr & FD_MSR_NONDMA) {
>>>> -        pos %= FD_SECTOR_LEN;
>>>>            if (pos == 0) {
>>>>                if (fdctrl->data_pos != 0)
>>>>                    if (!fdctrl_seek_to_next_sect(fdctrl, cur_drv)) {
>>>> @@ -1852,10 +1852,13 @@ static void fdctrl_handle_option(FDCtrl
>>>> *fdctrl, int direction)
>>>>    static void fdctrl_handle_drive_specification_command(FDCtrl
>>>> *fdctrl, int direction)
>>>>    {
>>>>        FDrive *cur_drv = get_cur_drv(fdctrl);
>>>> +    uint32_t pos;
>>>> -    if (fdctrl->fifo[fdctrl->data_pos - 1] & 0x80) {
>>>> +    pos = fdctrl->data_pos - 1;
>>>> +    pos %= FD_SECTOR_LEN;
>>>
>>> Shorter (and more clear):
>>>
>>> uint32_t pos = (fdctrl->data_pos - 1) % fdctrl->fifo_size;
>>>
>>>> +    if (fdctrl->fifo[pos] & 0x80) {
>>>>            /* Command parameters done */
>>>> -        if (fdctrl->fifo[fdctrl->data_pos - 1] & 0x40) {
>>>> +        if (fdctrl->fifo[pos] & 0x40) {
>>>>                fdctrl->fifo[0] = fdctrl->fifo[1];
>>>>                fdctrl->fifo[2] = 0;
>>>>                fdctrl->fifo[3] = 0;
>>>> @@ -1955,7 +1958,7 @@ static uint8_t command_to_handler[256];
>>>>    static void fdctrl_write_data(FDCtrl *fdctrl, uint32_t value)
>>>>    {
>>>>        FDrive *cur_drv;
>>>> -    int pos;
>>>> +    uint32_t pos;
>>>>        /* Reset mode */
>>>>        if (!(fdctrl->dor & FD_DOR_nRESET)) {
>>>> @@ -2004,7 +2007,9 @@ static void fdctrl_write_data(FDCtrl *fdctrl,
>>>> uint32_t value)
>>>>        }
>>>>        FLOPPY_DPRINTF("%s: %02x\n", __func__, value);
>>>> -    fdctrl->fifo[fdctrl->data_pos++] = value;
>>>> +    pos = fdctrl->data_pos++;
>>>> +    pos %= FD_SECTOR_LEN;
>>>> +    fdctrl->fifo[pos] = value;
>>>>        if (fdctrl->data_pos == fdctrl->data_len) {
>>>>            /* We now have all parameters
>>>>             * and will be able to treat the command
>>>
>>> Not strictly related to this patch: The code which sets fifo_size could
>>> also be improved.
>>>
>>>       fdctrl->fifo = qemu_memalign(512, FD_SECTOR_LEN);
>>>       fdctrl->fifo_size = 512;
>>>
>>> The 2nd line should be
>>>
>>>       fdctrl->fifo_size = FD_SECTOR_LEN;
>>>
>>>
>>> As far as I see the original code can read or write illegal memory
>>> locations in the address space of the QEMU process. It cannot (as it was
>>> claimed) modify the code of the VM host because those memory is usually
>>> write protected - at least if QEMU is running without KVM. If the code
>>> which is generated for KVM is writable from anywhere in QEMU, we should
>>> perhaps consider changing that.
>>>
>>> Regards
>>> Stefan
>>>
>>>
>>
>

Re: [Qemu-devel] [Qemu-stable] [PATCH] fdc: force the fifo access to be in bounds of the allocated buffer

[Stefan Priebe]

Am 13.05.2015 um 21:05 schrieb Stefan Weil:
> Am 13.05.2015 um 20:59 schrieb Stefan Priebe:
>>
>> Am 13.05.2015 um 20:51 schrieb Stefan Weil:
>>> Hi,
>>>
>>> I just noticed this patch because my provider told me that my KVM based
>>> server
>>> needs a reboot because of a CVE (see this German news:
>>> http://www.heise.de/newsticker/meldung/Venom-Schwachstelle-Aus-Hypervisor-ausbrechen-und-VMs-ausspionieren-2649614.html)
>>>
>>
>> Isn't a live migration to a fixed version enough instead of a reboot?
>>
>> Stefan
>
> Good point. A live migration would be sufficient - if there are no bugs
> in QEMU's live migration.

just migrating all our customer machines and wanted to be sure that live 
migration is enough.

Greets,
Stefan

>
> Stefan

Re: [Qemu-devel] [Qemu-stable] [PATCH] fdc: force the fifo access to be in bounds of the allocated buffer

[Peter Lieven]

Am 13.05.2015 um 21:09 schrieb Stefan Priebe:
> Am 13.05.2015 um 21:05 schrieb Stefan Weil:
>> Am 13.05.2015 um 20:59 schrieb Stefan Priebe:
>>>
>>> Am 13.05.2015 um 20:51 schrieb Stefan Weil:
>>>> Hi,
>>>>
>>>> I just noticed this patch because my provider told me that my KVM based
>>>> server
>>>> needs a reboot because of a CVE (see this German news:
>>>> http://www.heise.de/newsticker/meldung/Venom-Schwachstelle-Aus-Hypervisor-ausbrechen-und-VMs-ausspionieren-2649614.html)
>>>>
>>>
>>> Isn't a live migration to a fixed version enough instead of a reboot?
>>>
>>> Stefan
>>
>> Good point. A live migration would be sufficient - if there are no bugs
>> in QEMU's live migration.
>
> just migrating all our customer machines and wanted to be sure that live migration is enough.

Just to confirm: If Qemu is started with -nodefaults and there is no fdc configuration the system is not affected by this CVE?

Thanks,
Peter

Re: [Qemu-devel] [Qemu-stable] [PATCH] fdc: force the fifo access to be in bounds of the allocated buffer

[Markus Armbruster]

Peter Lieven <pl@kamp.de> writes:

> Am 13.05.2015 um 21:09 schrieb Stefan Priebe:
>> Am 13.05.2015 um 21:05 schrieb Stefan Weil:
>>> Am 13.05.2015 um 20:59 schrieb Stefan Priebe:
>>>>
>>>> Am 13.05.2015 um 20:51 schrieb Stefan Weil:
>>>>> Hi,
>>>>>
>>>>> I just noticed this patch because my provider told me that my KVM based
>>>>> server
>>>>> needs a reboot because of a CVE (see this German news:
>>>>> http://www.heise.de/newsticker/meldung/Venom-Schwachstelle-Aus-Hypervisor-ausbrechen-und-VMs-ausspionieren-2649614.html)
>>>>>
>>>>
>>>> Isn't a live migration to a fixed version enough instead of a reboot?
>>>>
>>>> Stefan
>>>
>>> Good point. A live migration would be sufficient - if there are no bugs
>>> in QEMU's live migration.
>>
>> just migrating all our customer machines and wanted to be sure that
>> live migration is enough.
>
> Just to confirm: If Qemu is started with -nodefaults and there is no
> fdc configuration the system is not affected by this CVE?

Not true.  The FD controller is still there.  It has no drives attached
then, but is vulnerable all the same.

Re: [Qemu-devel] [Qemu-stable] [PATCH] fdc: force the fifo access to be in bounds of the allocated buffer

[Peter Lieven]

Am 13.05.2015 um 21:52 schrieb Markus Armbruster:
> Peter Lieven <pl@kamp.de> writes:
>
>> Am 13.05.2015 um 21:09 schrieb Stefan Priebe:
>>> Am 13.05.2015 um 21:05 schrieb Stefan Weil:
>>>> Am 13.05.2015 um 20:59 schrieb Stefan Priebe:
>>>>> Am 13.05.2015 um 20:51 schrieb Stefan Weil:
>>>>>> Hi,
>>>>>>
>>>>>> I just noticed this patch because my provider told me that my KVM based
>>>>>> server
>>>>>> needs a reboot because of a CVE (see this German news:
>>>>>> http://www.heise.de/newsticker/meldung/Venom-Schwachstelle-Aus-Hypervisor-ausbrechen-und-VMs-ausspionieren-2649614.html)
>>>>>>
>>>>> Isn't a live migration to a fixed version enough instead of a reboot?
>>>>>
>>>>> Stefan
>>>> Good point. A live migration would be sufficient - if there are no bugs
>>>> in QEMU's live migration.
>>> just migrating all our customer machines and wanted to be sure that
>>> live migration is enough.
>> Just to confirm: If Qemu is started with -nodefaults and there is no
>> fdc configuration the system is not affected by this CVE?
> Not true.  The FD controller is still there.  It has no drives attached
> then, but is vulnerable all the same.

Are you sure? With -nodefaults the hmp command 'info block' returns nothing and
the guest sees no floppy drive.

Without -nodefaults I indeed see floppy0 and I have /dev/fd0 in the guest respectively.

Peter

Re: [Qemu-devel] [Qemu-stable] [PATCH] fdc: force the fifo access to be in bounds of the allocated buffer

[John Snow]

On 05/13/2015 04:02 PM, Peter Lieven wrote:
> Am 13.05.2015 um 21:52 schrieb Markus Armbruster:
>> Peter Lieven <pl@kamp.de> writes:
>>
>>> Am 13.05.2015 um 21:09 schrieb Stefan Priebe:
>>>> Am 13.05.2015 um 21:05 schrieb Stefan Weil:
>>>>> Am 13.05.2015 um 20:59 schrieb Stefan Priebe:
>>>>>> Am 13.05.2015 um 20:51 schrieb Stefan Weil:
>>>>>>> Hi,
>>>>>>>
>>>>>>> I just noticed this patch because my provider told me that my KVM based
>>>>>>> server
>>>>>>> needs a reboot because of a CVE (see this German news:
>>>>>>> http://www.heise.de/newsticker/meldung/Venom-Schwachstelle-Aus-Hypervisor-ausbrechen-und-VMs-ausspionieren-2649614.html)
>>>>>>>
>>>>>> Isn't a live migration to a fixed version enough instead of a reboot?
>>>>>>
>>>>>> Stefan
>>>>> Good point. A live migration would be sufficient - if there are no bugs
>>>>> in QEMU's live migration.
>>>> just migrating all our customer machines and wanted to be sure that
>>>> live migration is enough.
>>> Just to confirm: If Qemu is started with -nodefaults and there is no
>>> fdc configuration the system is not affected by this CVE?
>> Not true.  The FD controller is still there.  It has no drives attached
>> then, but is vulnerable all the same.
> 
> Are you sure? With -nodefaults the hmp command 'info block' returns nothing and
> the guest sees no floppy drive.
> 
> Without -nodefaults I indeed see floppy0 and I have /dev/fd0 in the guest respectively.
> 
> Peter
> 

It's not the *drive* that is the problem, it is the *controller*.

Re: [Qemu-devel] [Qemu-stable] [PATCH] fdc: force the fifo access to be in bounds of the allocated buffer

[Peter Lieven]

Am 13.05.2015 um 22:03 schrieb John Snow:
>
> On 05/13/2015 04:02 PM, Peter Lieven wrote:
>> Am 13.05.2015 um 21:52 schrieb Markus Armbruster:
>>> Peter Lieven <pl@kamp.de> writes:
>>>
>>>> Am 13.05.2015 um 21:09 schrieb Stefan Priebe:
>>>>> Am 13.05.2015 um 21:05 schrieb Stefan Weil:
>>>>>> Am 13.05.2015 um 20:59 schrieb Stefan Priebe:
>>>>>>> Am 13.05.2015 um 20:51 schrieb Stefan Weil:
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> I just noticed this patch because my provider told me that my KVM based
>>>>>>>> server
>>>>>>>> needs a reboot because of a CVE (see this German news:
>>>>>>>> http://www.heise.de/newsticker/meldung/Venom-Schwachstelle-Aus-Hypervisor-ausbrechen-und-VMs-ausspionieren-2649614.html)
>>>>>>>>
>>>>>>> Isn't a live migration to a fixed version enough instead of a reboot?
>>>>>>>
>>>>>>> Stefan
>>>>>> Good point. A live migration would be sufficient - if there are no bugs
>>>>>> in QEMU's live migration.
>>>>> just migrating all our customer machines and wanted to be sure that
>>>>> live migration is enough.
>>>> Just to confirm: If Qemu is started with -nodefaults and there is no
>>>> fdc configuration the system is not affected by this CVE?
>>> Not true.  The FD controller is still there.  It has no drives attached
>>> then, but is vulnerable all the same.
>> Are you sure? With -nodefaults the hmp command 'info block' returns nothing and
>> the guest sees no floppy drive.
>>
>> Without -nodefaults I indeed see floppy0 and I have /dev/fd0 in the guest respectively.
>>
>> Peter
>>
> It's not the *drive* that is the problem, it is the *controller*.
>
Okay, and indeed in the qtree I see the isa-fdc.

Thanks for sorting that out.

Thank you,
Peter

Re: [Qemu-devel] [PATCH] fdc: force the fifo access to be in bounds of the allocated buffer

[John Snow]

On 05/13/2015 02:51 PM, Stefan Weil wrote:
> Hi,
> 
> I just noticed this patch because my provider told me that my KVM based
> server
> needs a reboot because of a CVE (see this German news:
> http://www.heise.de/newsticker/meldung/Venom-Schwachstelle-Aus-Hypervisor-ausbrechen-und-VMs-ausspionieren-2649614.html)
> 
> 
> Am 13.05.2015 um 16:33 schrieb John Snow:
>> From: Petr Matousek <pmatouse@redhat.com>
>>
>> During processing of certain commands such as FD_CMD_READ_ID and
>> FD_CMD_DRIVE_SPECIFICATION_COMMAND the fifo memory access could
>> get out of bounds leading to memory corruption with values coming
>> from the guest.
>>
>> Fix this by making sure that the index is always bounded by the
>> allocated memory.
>>
>> This is CVE-2015-3456.
>>
>> Signed-off-by: Petr Matousek <pmatouse@redhat.com>
>> Reviewed-by: John Snow <jsnow@redhat.com>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>   hw/block/fdc.c | 17 +++++++++++------
>>   1 file changed, 11 insertions(+), 6 deletions(-)
>>
>> diff --git a/hw/block/fdc.c b/hw/block/fdc.c
>> index f72a392..d8a8edd 100644
>> --- a/hw/block/fdc.c
>> +++ b/hw/block/fdc.c
>> @@ -1497,7 +1497,7 @@ static uint32_t fdctrl_read_data(FDCtrl *fdctrl)
>>   {
>>       FDrive *cur_drv;
>>       uint32_t retval = 0;
>> -    int pos;
>> +    uint32_t pos;
>>         cur_drv = get_cur_drv(fdctrl);
>>       fdctrl->dsr &= ~FD_DSR_PWRDOWN;
>> @@ -1506,8 +1506,8 @@ static uint32_t fdctrl_read_data(FDCtrl *fdctrl)
>>           return 0;
>>       }
>>       pos = fdctrl->data_pos;
>> +    pos %= FD_SECTOR_LEN;
> 
> I'd combine both statements and perhaps use fdctrl->fifo_size (even if
> the resulting code will be slightly larger):
> 

Sure. Send me a patch and I'll ACK it.

> pos = fdctrl->data_pos % fdctrl->fifo_size;
> 
> 
>>       if (fdctrl->msr & FD_MSR_NONDMA) {
>> -        pos %= FD_SECTOR_LEN;
>>           if (pos == 0) {
>>               if (fdctrl->data_pos != 0)
>>                   if (!fdctrl_seek_to_next_sect(fdctrl, cur_drv)) {
>> @@ -1852,10 +1852,13 @@ static void fdctrl_handle_option(FDCtrl
>> *fdctrl, int direction)
>>   static void fdctrl_handle_drive_specification_command(FDCtrl
>> *fdctrl, int direction)
>>   {
>>       FDrive *cur_drv = get_cur_drv(fdctrl);
>> +    uint32_t pos;
>>   -    if (fdctrl->fifo[fdctrl->data_pos - 1] & 0x80) {
>> +    pos = fdctrl->data_pos - 1;
>> +    pos %= FD_SECTOR_LEN;
> 
> Shorter (and more clear):
> 
> uint32_t pos = (fdctrl->data_pos - 1) % fdctrl->fifo_size;
> 

Good here, too.

>> +    if (fdctrl->fifo[pos] & 0x80) {
>>           /* Command parameters done */
>> -        if (fdctrl->fifo[fdctrl->data_pos - 1] & 0x40) {
>> +        if (fdctrl->fifo[pos] & 0x40) {
>>               fdctrl->fifo[0] = fdctrl->fifo[1];
>>               fdctrl->fifo[2] = 0;
>>               fdctrl->fifo[3] = 0;
>> @@ -1955,7 +1958,7 @@ static uint8_t command_to_handler[256];
>>   static void fdctrl_write_data(FDCtrl *fdctrl, uint32_t value)
>>   {
>>       FDrive *cur_drv;
>> -    int pos;
>> +    uint32_t pos;
>>         /* Reset mode */
>>       if (!(fdctrl->dor & FD_DOR_nRESET)) {
>> @@ -2004,7 +2007,9 @@ static void fdctrl_write_data(FDCtrl *fdctrl,
>> uint32_t value)
>>       }
>>         FLOPPY_DPRINTF("%s: %02x\n", __func__, value);
>> -    fdctrl->fifo[fdctrl->data_pos++] = value;
>> +    pos = fdctrl->data_pos++;
>> +    pos %= FD_SECTOR_LEN;
>> +    fdctrl->fifo[pos] = value;
>>       if (fdctrl->data_pos == fdctrl->data_len) {
>>           /* We now have all parameters
>>            * and will be able to treat the command
> 
> Not strictly related to this patch: The code which sets fifo_size could
> also be improved.
> 
>     fdctrl->fifo = qemu_memalign(512, FD_SECTOR_LEN);
>     fdctrl->fifo_size = 512;
> 
> The 2nd line should be
> 
>     fdctrl->fifo_size = FD_SECTOR_LEN;
> 

Agreed, and it came up during the review for this, but we kept it out to
keep this a one patch targeted fix.

Also arising from the review: I want to move tmpbuf off of the stack,
though that particular buffer appears to be properly bounded at all times.

> 
> As far as I see the original code can read or write illegal memory
> locations in the address space of the QEMU process. It cannot (as it was
> claimed) modify the code of the VM host because those memory is usually
> write protected - at least if QEMU is running without KVM. If the code
> which is generated for KVM is writable from anywhere in QEMU, we should
> perhaps consider changing that.
> 

I don't think we are aware of any particular weaknesses, the security
report only said the "possibility" of arbitrary code execution due to
the buffer overflow. I haven't heard any more detailed explanation than
this.

> Regards
> Stefan
> 

Thanks,
--js