diff mbox

[2/2] pci-assign: Fix potential read beyond buffer on -EBUSY

Message ID 1393000925-8446-3-git-send-email-armbru@redhat.com
State New
Headers show

Commit Message

Markus Armbruster Feb. 21, 2014, 4:42 p.m. UTC 
readlink() doesn't write a terminating null byte.
assign_failed_examine() passes the unterminated string to strrchr().
Oops.  Terminate it.

Spotted by Coverity.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/i386/kvm/pci-assign.c | 1 +
 1 file changed, 1 insertion(+)

Comments

Peter Maydell Feb. 21, 2014, 4:51 p.m. UTC | #1 
On 21 February 2014 16:42, Markus Armbruster <armbru@redhat.com> wrote:
> readlink() doesn't write a terminating null byte.
> assign_failed_examine() passes the unterminated string to strrchr().
> Oops.  Terminate it.
>
> Spotted by Coverity.
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  hw/i386/kvm/pci-assign.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/hw/i386/kvm/pci-assign.c b/hw/i386/kvm/pci-assign.c
> index 9686801..a825871 100644
> --- a/hw/i386/kvm/pci-assign.c
> +++ b/hw/i386/kvm/pci-assign.c
> @@ -743,6 +743,7 @@ static void assign_failed_examine(AssignedDevice *dev)
>          goto fail;
>      }
>
> +    driver[r] = 0;

This will write off the end of the buffer if readlink()
filled it completely, won't it? I think you also need
to change the readlink() 3rd argument to "sizeof(driver) - 1".

thanks
-- PMM
Peter Maydell Feb. 21, 2014, 4:55 p.m. UTC | #2 
On 21 February 2014 16:51, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 21 February 2014 16:42, Markus Armbruster <armbru@redhat.com> wrote:
>> readlink() doesn't write a terminating null byte.
>> assign_failed_examine() passes the unterminated string to strrchr().
>> Oops.  Terminate it.
>>
>> Spotted by Coverity.
>>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  hw/i386/kvm/pci-assign.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/hw/i386/kvm/pci-assign.c b/hw/i386/kvm/pci-assign.c
>> index 9686801..a825871 100644
>> --- a/hw/i386/kvm/pci-assign.c
>> +++ b/hw/i386/kvm/pci-assign.c
>> @@ -743,6 +743,7 @@ static void assign_failed_examine(AssignedDevice *dev)
>>          goto fail;
>>      }
>>
>> +    driver[r] = 0;
>
> This will write off the end of the buffer if readlink()
> filled it completely, won't it? I think you also need
> to change the readlink() 3rd argument to "sizeof(driver) - 1".

Apologies for this aspersion -- we check for
"r ==  sizeof(driver)" and bail out before we get here,
so the patch is ok.

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM
diff mbox

Patch

diff --git a/hw/i386/kvm/pci-assign.c b/hw/i386/kvm/pci-assign.c
index 9686801..a825871 100644
--- a/hw/i386/kvm/pci-assign.c
+++ b/hw/i386/kvm/pci-assign.c
@@ -743,6 +743,7 @@  static void assign_failed_examine(AssignedDevice *dev)
         goto fail;
     }
 
+    driver[r] = 0;
     ns = strrchr(driver, '/');
     if (!ns) {
         goto fail;