[Linaro-uefi] Drivers: MarvellYukonDxe driver binding fixes

Message ID 20170412204705.28961-1-leif.lindholm@linaro.org
State New
Headers show

Commit Message

Leif Lindholm April 12, 2017, 8:47 p.m.
Fix a case where MarvellYukonDriverStart attempted to free a buffer
before it was allocated.

Fix another case where the function returned without freeing the same
buffer. Also correct the error message for that case, which was copied
verbatim from the preceding if-statement.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>
---
 Drivers/Net/MarvellYukonDxe/DriverBinding.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Alan Ott April 12, 2017, 9:16 p.m. | #1
Tested-by: Alan Ott <alan@softiron.co.uk>

On 04/12/2017 04:47 PM, Leif Lindholm wrote:
> Fix a case where MarvellYukonDriverStart attempted to free a buffer
> before it was allocated.
>
> Fix another case where the function returned without freeing the same
> buffer. Also correct the error message for that case, which was copied
> verbatim from the preceding if-statement.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>
> ---
>   Drivers/Net/MarvellYukonDxe/DriverBinding.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/Drivers/Net/MarvellYukonDxe/DriverBinding.c b/Drivers/Net/MarvellYukonDxe/DriverBinding.c
> index 947d738b10..cbe8ad34e4 100644
> --- a/Drivers/Net/MarvellYukonDxe/DriverBinding.c
> +++ b/Drivers/Net/MarvellYukonDxe/DriverBinding.c
> @@ -127,7 +127,6 @@ MarvellYukonDriverStart (
>   
>     if (EFI_ERROR (Status)) {
>       DEBUG ((EFI_D_ERROR, "Marvell Yukon: OpenProtocol: EFI_PCI_IO_PROTOCOL ERROR Status = %r\n", Status));
> -    gBS->FreePool (YukonDriver);
>       return Status;
>     }
>   
> @@ -156,7 +155,8 @@ MarvellYukonDriverStart (
>       }
>   
>       if (ScData->msk_if[Port] == NULL) {
> -      DEBUG ((DEBUG_ERROR, "Marvell Yukon: AllocatePool() failed with Status = %r\n", EFI_BAD_BUFFER_SIZE));
> +      DEBUG ((DEBUG_ERROR, "Marvell Yukon: invalid buffer size\n"));
> +      gBS->FreePool (YukonDriver);
>         return EFI_BAD_BUFFER_SIZE;
>       }
>
Daniil Egranov April 13, 2017, 2:04 a.m. | #2
Tested-by: Daniil Egranov <daniil.egranov@arm.com>



On 04/12/2017 03:47 PM, Leif Lindholm wrote:
> Fix a case where MarvellYukonDriverStart attempted to free a buffer

> before it was allocated.

>

> Fix another case where the function returned without freeing the same

> buffer. Also correct the error message for that case, which was copied

> verbatim from the preceding if-statement.

>

> Contributed-under: TianoCore Contribution Agreement 1.0

> Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>

> ---

>   Drivers/Net/MarvellYukonDxe/DriverBinding.c | 4 ++--

>   1 file changed, 2 insertions(+), 2 deletions(-)

>

> diff --git a/Drivers/Net/MarvellYukonDxe/DriverBinding.c b/Drivers/Net/MarvellYukonDxe/DriverBinding.c

> index 947d738b10..cbe8ad34e4 100644

> --- a/Drivers/Net/MarvellYukonDxe/DriverBinding.c

> +++ b/Drivers/Net/MarvellYukonDxe/DriverBinding.c

> @@ -127,7 +127,6 @@ MarvellYukonDriverStart (

>   

>     if (EFI_ERROR (Status)) {

>       DEBUG ((EFI_D_ERROR, "Marvell Yukon: OpenProtocol: EFI_PCI_IO_PROTOCOL ERROR Status = %r\n", Status));

> -    gBS->FreePool (YukonDriver);

>       return Status;

>     }

>   

> @@ -156,7 +155,8 @@ MarvellYukonDriverStart (

>       }

>   

>       if (ScData->msk_if[Port] == NULL) {

> -      DEBUG ((DEBUG_ERROR, "Marvell Yukon: AllocatePool() failed with Status = %r\n", EFI_BAD_BUFFER_SIZE));

> +      DEBUG ((DEBUG_ERROR, "Marvell Yukon: invalid buffer size\n"));

> +      gBS->FreePool (YukonDriver);

>         return EFI_BAD_BUFFER_SIZE;

>       }

>
Ard Biesheuvel April 13, 2017, 7:29 a.m. | #3
On 12 April 2017 at 21:47, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> Fix a case where MarvellYukonDriverStart attempted to free a buffer
> before it was allocated.
>
> Fix another case where the function returned without freeing the same
> buffer. Also correct the error message for that case, which was copied
> verbatim from the preceding if-statement.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>
> ---
>  Drivers/Net/MarvellYukonDxe/DriverBinding.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/Drivers/Net/MarvellYukonDxe/DriverBinding.c b/Drivers/Net/MarvellYukonDxe/DriverBinding.c
> index 947d738b10..cbe8ad34e4 100644
> --- a/Drivers/Net/MarvellYukonDxe/DriverBinding.c
> +++ b/Drivers/Net/MarvellYukonDxe/DriverBinding.c
> @@ -127,7 +127,6 @@ MarvellYukonDriverStart (
>
>    if (EFI_ERROR (Status)) {
>      DEBUG ((EFI_D_ERROR, "Marvell Yukon: OpenProtocol: EFI_PCI_IO_PROTOCOL ERROR Status = %r\n", Status));
> -    gBS->FreePool (YukonDriver);
>      return Status;
>    }
>
> @@ -156,7 +155,8 @@ MarvellYukonDriverStart (
>      }
>
>      if (ScData->msk_if[Port] == NULL) {
> -      DEBUG ((DEBUG_ERROR, "Marvell Yukon: AllocatePool() failed with Status = %r\n", EFI_BAD_BUFFER_SIZE));
> +      DEBUG ((DEBUG_ERROR, "Marvell Yukon: invalid buffer size\n"));
> +      gBS->FreePool (YukonDriver);
>        return EFI_BAD_BUFFER_SIZE;
>      }
>

Wouldn't it make much more sense to move this test above the AllocatePool() ?
Leif Lindholm April 13, 2017, 1:09 p.m. | #4
On Thu, Apr 13, 2017 at 08:29:01AM +0100, Ard Biesheuvel wrote:
> On 12 April 2017 at 21:47, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> > Fix a case where MarvellYukonDriverStart attempted to free a buffer
> > before it was allocated.
> >
> > Fix another case where the function returned without freeing the same
> > buffer. Also correct the error message for that case, which was copied
> > verbatim from the preceding if-statement.
> >
> > Contributed-under: TianoCore Contribution Agreement 1.0
> > Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>
> > ---
> >  Drivers/Net/MarvellYukonDxe/DriverBinding.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/Drivers/Net/MarvellYukonDxe/DriverBinding.c b/Drivers/Net/MarvellYukonDxe/DriverBinding.c
> > index 947d738b10..cbe8ad34e4 100644
> > --- a/Drivers/Net/MarvellYukonDxe/DriverBinding.c
> > +++ b/Drivers/Net/MarvellYukonDxe/DriverBinding.c
> > @@ -127,7 +127,6 @@ MarvellYukonDriverStart (
> >
> >    if (EFI_ERROR (Status)) {
> >      DEBUG ((EFI_D_ERROR, "Marvell Yukon: OpenProtocol: EFI_PCI_IO_PROTOCOL ERROR Status = %r\n", Status));
> > -    gBS->FreePool (YukonDriver);
> >      return Status;
> >    }
> >
> > @@ -156,7 +155,8 @@ MarvellYukonDriverStart (
> >      }
> >
> >      if (ScData->msk_if[Port] == NULL) {
> > -      DEBUG ((DEBUG_ERROR, "Marvell Yukon: AllocatePool() failed with Status = %r\n", EFI_BAD_BUFFER_SIZE));
> > +      DEBUG ((DEBUG_ERROR, "Marvell Yukon: invalid buffer size\n"));
> > +      gBS->FreePool (YukonDriver);
> >        return EFI_BAD_BUFFER_SIZE;
> >      }
> >
> 
> Wouldn't it make much more sense to move this test above the AllocatePool() ?

You're not wrong.

How about this?:

From 977ad2b9b4ae02c2e1995279bd49c02f86ccd10a Mon Sep 17 00:00:00 2001
From: Leif Lindholm <leif.lindholm@linaro.org>
Date: Wed, 12 Apr 2017 15:23:47 +0100
Subject: [PATCH] Drivers: MarvellYukonDxe driver binding fixes

Fix a case where MarvellYukonDriverStart attempted to free a buffer
before it was allocated.

Move another error case which wasn't freeing the same buffer before
returning. Also correct the error message for that case, which was
copied verbatim from the preceding if-statement.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>
---
 Drivers/Net/MarvellYukonDxe/DriverBinding.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)diff --git a/Drivers/Net/MarvellYukonDxe/DriverBinding.c b/Drivers/Net/MarvellYukonDxe/DriverBinding.c
index 947d738..0abc94e 100644
--- a/Drivers/Net/MarvellYukonDxe/DriverBinding.c
+++ b/Drivers/Net/MarvellYukonDxe/DriverBinding.c
@@ -127,7 +127,6 @@ MarvellYukonDriverStart (
 
   if (EFI_ERROR (Status)) {
     DEBUG ((EFI_D_ERROR, "Marvell Yukon: OpenProtocol: EFI_PCI_IO_PROTOCOL ERROR Status = %r\n", Status));
-    gBS->FreePool (YukonDriver);
     return Status;
   }
 
@@ -146,6 +145,10 @@ MarvellYukonDriverStart (
   }
 
   for (Port = 0; Port < ScData->msk_num_port; Port++) {
+    if (ScData->msk_if[Port] == NULL) {
+      DEBUG ((DEBUG_ERROR, "Marvell Yukon: invalid buffer size\n"));
+      return EFI_BAD_BUFFER_SIZE;
+    }
 
     Status = gBS->AllocatePool (EfiBootServicesData,
                                 sizeof (YUKON_DRIVER),
@@ -155,11 +158,6 @@ MarvellYukonDriverStart (
       return Status;
     }
 
-    if (ScData->msk_if[Port] == NULL) {
-      DEBUG ((DEBUG_ERROR, "Marvell Yukon: AllocatePool() failed with Status = %r\n", EFI_BAD_BUFFER_SIZE));
-      return EFI_BAD_BUFFER_SIZE;
-    }
-
     gBS->SetMem (YukonDriver, sizeof (YUKON_DRIVER), 0);
     EfiInitializeLock (&YukonDriver->Lock, TPL_NOTIFY);
 

Ard Biesheuvel April 13, 2017, 1:13 p.m. | #5
On 13 April 2017 at 14:09, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> On Thu, Apr 13, 2017 at 08:29:01AM +0100, Ard Biesheuvel wrote:
>> On 12 April 2017 at 21:47, Leif Lindholm <leif.lindholm@linaro.org> wrote:
>> > Fix a case where MarvellYukonDriverStart attempted to free a buffer
>> > before it was allocated.
>> >
>> > Fix another case where the function returned without freeing the same
>> > buffer. Also correct the error message for that case, which was copied
>> > verbatim from the preceding if-statement.
>> >
>> > Contributed-under: TianoCore Contribution Agreement 1.0
>> > Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>
>> > ---
>> >  Drivers/Net/MarvellYukonDxe/DriverBinding.c | 4 ++--
>> >  1 file changed, 2 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/Drivers/Net/MarvellYukonDxe/DriverBinding.c b/Drivers/Net/MarvellYukonDxe/DriverBinding.c
>> > index 947d738b10..cbe8ad34e4 100644
>> > --- a/Drivers/Net/MarvellYukonDxe/DriverBinding.c
>> > +++ b/Drivers/Net/MarvellYukonDxe/DriverBinding.c
>> > @@ -127,7 +127,6 @@ MarvellYukonDriverStart (
>> >
>> >    if (EFI_ERROR (Status)) {
>> >      DEBUG ((EFI_D_ERROR, "Marvell Yukon: OpenProtocol: EFI_PCI_IO_PROTOCOL ERROR Status = %r\n", Status));
>> > -    gBS->FreePool (YukonDriver);
>> >      return Status;
>> >    }
>> >
>> > @@ -156,7 +155,8 @@ MarvellYukonDriverStart (
>> >      }
>> >
>> >      if (ScData->msk_if[Port] == NULL) {
>> > -      DEBUG ((DEBUG_ERROR, "Marvell Yukon: AllocatePool() failed with Status = %r\n", EFI_BAD_BUFFER_SIZE));
>> > +      DEBUG ((DEBUG_ERROR, "Marvell Yukon: invalid buffer size\n"));
>> > +      gBS->FreePool (YukonDriver);
>> >        return EFI_BAD_BUFFER_SIZE;
>> >      }
>> >
>>
>> Wouldn't it make much more sense to move this test above the AllocatePool() ?
>
> You're not wrong.
>
> How about this?:
>
> From 977ad2b9b4ae02c2e1995279bd49c02f86ccd10a Mon Sep 17 00:00:00 2001
> From: Leif Lindholm <leif.lindholm@linaro.org>
> Date: Wed, 12 Apr 2017 15:23:47 +0100
> Subject: [PATCH] Drivers: MarvellYukonDxe driver binding fixes
>
> Fix a case where MarvellYukonDriverStart attempted to free a buffer
> before it was allocated.
>
> Move another error case which wasn't freeing the same buffer before
> returning. Also correct the error message for that case, which was
> copied verbatim from the preceding if-statement.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>
> ---
>  Drivers/Net/MarvellYukonDxe/DriverBinding.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/Drivers/Net/MarvellYukonDxe/DriverBinding.c b/Drivers/Net/MarvellYukonDxe/DriverBinding.c
> index 947d738..0abc94e 100644
> --- a/Drivers/Net/MarvellYukonDxe/DriverBinding.c
> +++ b/Drivers/Net/MarvellYukonDxe/DriverBinding.c
> @@ -127,7 +127,6 @@ MarvellYukonDriverStart (
>
>    if (EFI_ERROR (Status)) {
>      DEBUG ((EFI_D_ERROR, "Marvell Yukon: OpenProtocol: EFI_PCI_IO_PROTOCOL ERROR Status = %r\n", Status));
> -    gBS->FreePool (YukonDriver);
>      return Status;
>    }
>
> @@ -146,6 +145,10 @@ MarvellYukonDriverStart (
>    }
>
>    for (Port = 0; Port < ScData->msk_num_port; Port++) {
> +    if (ScData->msk_if[Port] == NULL) {
> +      DEBUG ((DEBUG_ERROR, "Marvell Yukon: invalid buffer size\n"));
> +      return EFI_BAD_BUFFER_SIZE;
> +    }
>
>      Status = gBS->AllocatePool (EfiBootServicesData,
>                                  sizeof (YUKON_DRIVER),
> @@ -155,11 +158,6 @@ MarvellYukonDriverStart (
>        return Status;
>      }
>
> -    if (ScData->msk_if[Port] == NULL) {
> -      DEBUG ((DEBUG_ERROR, "Marvell Yukon: AllocatePool() failed with Status = %r\n", EFI_BAD_BUFFER_SIZE));
> -      return EFI_BAD_BUFFER_SIZE;
> -    }
> -
>      gBS->SetMem (YukonDriver, sizeof (YUKON_DRIVER), 0);
>      EfiInitializeLock (&YukonDriver->Lock, TPL_NOTIFY);
>

Works for me

Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Leif Lindholm April 13, 2017, 1:39 p.m. | #6
Daniil, Alan - can I keep your Tested-by:s?

/
    Leif

On Thu, Apr 13, 2017 at 02:13:24PM +0100, Ard Biesheuvel wrote:
> On 13 April 2017 at 14:09, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> > On Thu, Apr 13, 2017 at 08:29:01AM +0100, Ard Biesheuvel wrote:
> >> On 12 April 2017 at 21:47, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> >> > Fix a case where MarvellYukonDriverStart attempted to free a buffer
> >> > before it was allocated.
> >> >
> >> > Fix another case where the function returned without freeing the same
> >> > buffer. Also correct the error message for that case, which was copied
> >> > verbatim from the preceding if-statement.
> >> >
> >> > Contributed-under: TianoCore Contribution Agreement 1.0
> >> > Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>
> >> > ---
> >> >  Drivers/Net/MarvellYukonDxe/DriverBinding.c | 4 ++--
> >> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >> >
> >> > diff --git a/Drivers/Net/MarvellYukonDxe/DriverBinding.c b/Drivers/Net/MarvellYukonDxe/DriverBinding.c
> >> > index 947d738b10..cbe8ad34e4 100644
> >> > --- a/Drivers/Net/MarvellYukonDxe/DriverBinding.c
> >> > +++ b/Drivers/Net/MarvellYukonDxe/DriverBinding.c
> >> > @@ -127,7 +127,6 @@ MarvellYukonDriverStart (
> >> >
> >> >    if (EFI_ERROR (Status)) {
> >> >      DEBUG ((EFI_D_ERROR, "Marvell Yukon: OpenProtocol: EFI_PCI_IO_PROTOCOL ERROR Status = %r\n", Status));
> >> > -    gBS->FreePool (YukonDriver);
> >> >      return Status;
> >> >    }
> >> >
> >> > @@ -156,7 +155,8 @@ MarvellYukonDriverStart (
> >> >      }
> >> >
> >> >      if (ScData->msk_if[Port] == NULL) {
> >> > -      DEBUG ((DEBUG_ERROR, "Marvell Yukon: AllocatePool() failed with Status = %r\n", EFI_BAD_BUFFER_SIZE));
> >> > +      DEBUG ((DEBUG_ERROR, "Marvell Yukon: invalid buffer size\n"));
> >> > +      gBS->FreePool (YukonDriver);
> >> >        return EFI_BAD_BUFFER_SIZE;
> >> >      }
> >> >
> >>
> >> Wouldn't it make much more sense to move this test above the AllocatePool() ?
> >
> > You're not wrong.
> >
> > How about this?:
> >
> > From 977ad2b9b4ae02c2e1995279bd49c02f86ccd10a Mon Sep 17 00:00:00 2001
> > From: Leif Lindholm <leif.lindholm@linaro.org>
> > Date: Wed, 12 Apr 2017 15:23:47 +0100
> > Subject: [PATCH] Drivers: MarvellYukonDxe driver binding fixes
> >
> > Fix a case where MarvellYukonDriverStart attempted to free a buffer
> > before it was allocated.
> >
> > Move another error case which wasn't freeing the same buffer before
> > returning. Also correct the error message for that case, which was
> > copied verbatim from the preceding if-statement.
> >
> > Contributed-under: TianoCore Contribution Agreement 1.0
> > Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>
> > ---
> >  Drivers/Net/MarvellYukonDxe/DriverBinding.c | 10 ++++------
> >  1 file changed, 4 insertions(+), 6 deletions(-)
> >
> > diff --git a/Drivers/Net/MarvellYukonDxe/DriverBinding.c b/Drivers/Net/MarvellYukonDxe/DriverBinding.c
> > index 947d738..0abc94e 100644
> > --- a/Drivers/Net/MarvellYukonDxe/DriverBinding.c
> > +++ b/Drivers/Net/MarvellYukonDxe/DriverBinding.c
> > @@ -127,7 +127,6 @@ MarvellYukonDriverStart (
> >
> >    if (EFI_ERROR (Status)) {
> >      DEBUG ((EFI_D_ERROR, "Marvell Yukon: OpenProtocol: EFI_PCI_IO_PROTOCOL ERROR Status = %r\n", Status));
> > -    gBS->FreePool (YukonDriver);
> >      return Status;
> >    }
> >
> > @@ -146,6 +145,10 @@ MarvellYukonDriverStart (
> >    }
> >
> >    for (Port = 0; Port < ScData->msk_num_port; Port++) {
> > +    if (ScData->msk_if[Port] == NULL) {
> > +      DEBUG ((DEBUG_ERROR, "Marvell Yukon: invalid buffer size\n"));
> > +      return EFI_BAD_BUFFER_SIZE;
> > +    }
> >
> >      Status = gBS->AllocatePool (EfiBootServicesData,
> >                                  sizeof (YUKON_DRIVER),
> > @@ -155,11 +158,6 @@ MarvellYukonDriverStart (
> >        return Status;
> >      }
> >
> > -    if (ScData->msk_if[Port] == NULL) {
> > -      DEBUG ((DEBUG_ERROR, "Marvell Yukon: AllocatePool() failed with Status = %r\n", EFI_BAD_BUFFER_SIZE));
> > -      return EFI_BAD_BUFFER_SIZE;
> > -    }
> > -
> >      gBS->SetMem (YukonDriver, sizeof (YUKON_DRIVER), 0);
> >      EfiInitializeLock (&YukonDriver->Lock, TPL_NOTIFY);
> >
> 
> Works for me
> 
> Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Alan Ott April 13, 2017, 1:49 p.m. | #7
That's fine.

On 04/13/2017 09:39 AM, Leif Lindholm wrote:
> Daniil, Alan - can I keep your Tested-by:s?
>
> /
>      Leif
>
> On Thu, Apr 13, 2017 at 02:13:24PM +0100, Ard Biesheuvel wrote:
>> On 13 April 2017 at 14:09, Leif Lindholm <leif.lindholm@linaro.org> wrote:
>>> On Thu, Apr 13, 2017 at 08:29:01AM +0100, Ard Biesheuvel wrote:
>>>> On 12 April 2017 at 21:47, Leif Lindholm <leif.lindholm@linaro.org> wrote:
>>>>> Fix a case where MarvellYukonDriverStart attempted to free a buffer
>>>>> before it was allocated.
>>>>>
>>>>> Fix another case where the function returned without freeing the same
>>>>> buffer. Also correct the error message for that case, which was copied
>>>>> verbatim from the preceding if-statement.
>>>>>
>>>>> Contributed-under: TianoCore Contribution Agreement 1.0
>>>>> Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>
>>>>> ---
>>>>>   Drivers/Net/MarvellYukonDxe/DriverBinding.c | 4 ++--
>>>>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/Drivers/Net/MarvellYukonDxe/DriverBinding.c b/Drivers/Net/MarvellYukonDxe/DriverBinding.c
>>>>> index 947d738b10..cbe8ad34e4 100644
>>>>> --- a/Drivers/Net/MarvellYukonDxe/DriverBinding.c
>>>>> +++ b/Drivers/Net/MarvellYukonDxe/DriverBinding.c
>>>>> @@ -127,7 +127,6 @@ MarvellYukonDriverStart (
>>>>>
>>>>>     if (EFI_ERROR (Status)) {
>>>>>       DEBUG ((EFI_D_ERROR, "Marvell Yukon: OpenProtocol: EFI_PCI_IO_PROTOCOL ERROR Status = %r\n", Status));
>>>>> -    gBS->FreePool (YukonDriver);
>>>>>       return Status;
>>>>>     }
>>>>>
>>>>> @@ -156,7 +155,8 @@ MarvellYukonDriverStart (
>>>>>       }
>>>>>
>>>>>       if (ScData->msk_if[Port] == NULL) {
>>>>> -      DEBUG ((DEBUG_ERROR, "Marvell Yukon: AllocatePool() failed with Status = %r\n", EFI_BAD_BUFFER_SIZE));
>>>>> +      DEBUG ((DEBUG_ERROR, "Marvell Yukon: invalid buffer size\n"));
>>>>> +      gBS->FreePool (YukonDriver);
>>>>>         return EFI_BAD_BUFFER_SIZE;
>>>>>       }
>>>>>
>>>> Wouldn't it make much more sense to move this test above the AllocatePool() ?
>>> You're not wrong.
>>>
>>> How about this?:
>>>
>>>  From 977ad2b9b4ae02c2e1995279bd49c02f86ccd10a Mon Sep 17 00:00:00 2001
>>> From: Leif Lindholm <leif.lindholm@linaro.org>
>>> Date: Wed, 12 Apr 2017 15:23:47 +0100
>>> Subject: [PATCH] Drivers: MarvellYukonDxe driver binding fixes
>>>
>>> Fix a case where MarvellYukonDriverStart attempted to free a buffer
>>> before it was allocated.
>>>
>>> Move another error case which wasn't freeing the same buffer before
>>> returning. Also correct the error message for that case, which was
>>> copied verbatim from the preceding if-statement.
>>>
>>> Contributed-under: TianoCore Contribution Agreement 1.0
>>> Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>
>>> ---
>>>   Drivers/Net/MarvellYukonDxe/DriverBinding.c | 10 ++++------
>>>   1 file changed, 4 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/Drivers/Net/MarvellYukonDxe/DriverBinding.c b/Drivers/Net/MarvellYukonDxe/DriverBinding.c
>>> index 947d738..0abc94e 100644
>>> --- a/Drivers/Net/MarvellYukonDxe/DriverBinding.c
>>> +++ b/Drivers/Net/MarvellYukonDxe/DriverBinding.c
>>> @@ -127,7 +127,6 @@ MarvellYukonDriverStart (
>>>
>>>     if (EFI_ERROR (Status)) {
>>>       DEBUG ((EFI_D_ERROR, "Marvell Yukon: OpenProtocol: EFI_PCI_IO_PROTOCOL ERROR Status = %r\n", Status));
>>> -    gBS->FreePool (YukonDriver);
>>>       return Status;
>>>     }
>>>
>>> @@ -146,6 +145,10 @@ MarvellYukonDriverStart (
>>>     }
>>>
>>>     for (Port = 0; Port < ScData->msk_num_port; Port++) {
>>> +    if (ScData->msk_if[Port] == NULL) {
>>> +      DEBUG ((DEBUG_ERROR, "Marvell Yukon: invalid buffer size\n"));
>>> +      return EFI_BAD_BUFFER_SIZE;
>>> +    }
>>>
>>>       Status = gBS->AllocatePool (EfiBootServicesData,
>>>                                   sizeof (YUKON_DRIVER),
>>> @@ -155,11 +158,6 @@ MarvellYukonDriverStart (
>>>         return Status;
>>>       }
>>>
>>> -    if (ScData->msk_if[Port] == NULL) {
>>> -      DEBUG ((DEBUG_ERROR, "Marvell Yukon: AllocatePool() failed with Status = %r\n", EFI_BAD_BUFFER_SIZE));
>>> -      return EFI_BAD_BUFFER_SIZE;
>>> -    }
>>> -
>>>       gBS->SetMem (YukonDriver, sizeof (YUKON_DRIVER), 0);
>>>       EfiInitializeLock (&YukonDriver->Lock, TPL_NOTIFY);
>>>
>> Works for me
>>
>> Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Daniil Egranov April 13, 2017, 2:17 p.m. | #8
Sure.

Thanks,
Daniil

On 04/13/2017 08:39 AM, Leif Lindholm wrote:
> Daniil, Alan - can I keep your Tested-by:s?
>
> /
>      Leif
>
> On Thu, Apr 13, 2017 at 02:13:24PM +0100, Ard Biesheuvel wrote:
>> On 13 April 2017 at 14:09, Leif Lindholm <leif.lindholm@linaro.org> wrote:
>>> On Thu, Apr 13, 2017 at 08:29:01AM +0100, Ard Biesheuvel wrote:
>>>> On 12 April 2017 at 21:47, Leif Lindholm <leif.lindholm@linaro.org> wrote:
>>>>> Fix a case where MarvellYukonDriverStart attempted to free a buffer
>>>>> before it was allocated.
>>>>>
>>>>> Fix another case where the function returned without freeing the same
>>>>> buffer. Also correct the error message for that case, which was copied
>>>>> verbatim from the preceding if-statement.
>>>>>
>>>>> Contributed-under: TianoCore Contribution Agreement 1.0
>>>>> Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>
>>>>> ---
>>>>>   Drivers/Net/MarvellYukonDxe/DriverBinding.c | 4 ++--
>>>>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/Drivers/Net/MarvellYukonDxe/DriverBinding.c b/Drivers/Net/MarvellYukonDxe/DriverBinding.c
>>>>> index 947d738b10..cbe8ad34e4 100644
>>>>> --- a/Drivers/Net/MarvellYukonDxe/DriverBinding.c
>>>>> +++ b/Drivers/Net/MarvellYukonDxe/DriverBinding.c
>>>>> @@ -127,7 +127,6 @@ MarvellYukonDriverStart (
>>>>>
>>>>>     if (EFI_ERROR (Status)) {
>>>>>       DEBUG ((EFI_D_ERROR, "Marvell Yukon: OpenProtocol: EFI_PCI_IO_PROTOCOL ERROR Status = %r\n", Status));
>>>>> -    gBS->FreePool (YukonDriver);
>>>>>       return Status;
>>>>>     }
>>>>>
>>>>> @@ -156,7 +155,8 @@ MarvellYukonDriverStart (
>>>>>       }
>>>>>
>>>>>       if (ScData->msk_if[Port] == NULL) {
>>>>> -      DEBUG ((DEBUG_ERROR, "Marvell Yukon: AllocatePool() failed with Status = %r\n", EFI_BAD_BUFFER_SIZE));
>>>>> +      DEBUG ((DEBUG_ERROR, "Marvell Yukon: invalid buffer size\n"));
>>>>> +      gBS->FreePool (YukonDriver);
>>>>>         return EFI_BAD_BUFFER_SIZE;
>>>>>       }
>>>>>
>>>> Wouldn't it make much more sense to move this test above the AllocatePool() ?
>>> You're not wrong.
>>>
>>> How about this?:
>>>
>>>  From 977ad2b9b4ae02c2e1995279bd49c02f86ccd10a Mon Sep 17 00:00:00 2001
>>> From: Leif Lindholm <leif.lindholm@linaro.org>
>>> Date: Wed, 12 Apr 2017 15:23:47 +0100
>>> Subject: [PATCH] Drivers: MarvellYukonDxe driver binding fixes
>>>
>>> Fix a case where MarvellYukonDriverStart attempted to free a buffer
>>> before it was allocated.
>>>
>>> Move another error case which wasn't freeing the same buffer before
>>> returning. Also correct the error message for that case, which was
>>> copied verbatim from the preceding if-statement.
>>>
>>> Contributed-under: TianoCore Contribution Agreement 1.0
>>> Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>
>>> ---
>>>   Drivers/Net/MarvellYukonDxe/DriverBinding.c | 10 ++++------
>>>   1 file changed, 4 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/Drivers/Net/MarvellYukonDxe/DriverBinding.c b/Drivers/Net/MarvellYukonDxe/DriverBinding.c
>>> index 947d738..0abc94e 100644
>>> --- a/Drivers/Net/MarvellYukonDxe/DriverBinding.c
>>> +++ b/Drivers/Net/MarvellYukonDxe/DriverBinding.c
>>> @@ -127,7 +127,6 @@ MarvellYukonDriverStart (
>>>
>>>     if (EFI_ERROR (Status)) {
>>>       DEBUG ((EFI_D_ERROR, "Marvell Yukon: OpenProtocol: EFI_PCI_IO_PROTOCOL ERROR Status = %r\n", Status));
>>> -    gBS->FreePool (YukonDriver);
>>>       return Status;
>>>     }
>>>
>>> @@ -146,6 +145,10 @@ MarvellYukonDriverStart (
>>>     }
>>>
>>>     for (Port = 0; Port < ScData->msk_num_port; Port++) {
>>> +    if (ScData->msk_if[Port] == NULL) {
>>> +      DEBUG ((DEBUG_ERROR, "Marvell Yukon: invalid buffer size\n"));
>>> +      return EFI_BAD_BUFFER_SIZE;
>>> +    }
>>>
>>>       Status = gBS->AllocatePool (EfiBootServicesData,
>>>                                   sizeof (YUKON_DRIVER),
>>> @@ -155,11 +158,6 @@ MarvellYukonDriverStart (
>>>         return Status;
>>>       }
>>>
>>> -    if (ScData->msk_if[Port] == NULL) {
>>> -      DEBUG ((DEBUG_ERROR, "Marvell Yukon: AllocatePool() failed with Status = %r\n", EFI_BAD_BUFFER_SIZE));
>>> -      return EFI_BAD_BUFFER_SIZE;
>>> -    }
>>> -
>>>       gBS->SetMem (YukonDriver, sizeof (YUKON_DRIVER), 0);
>>>       EfiInitializeLock (&YukonDriver->Lock, TPL_NOTIFY);
>>>
>> Works for me
>>
>> Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> _______________________________________________
> Linaro-uefi mailing list
> Linaro-uefi@lists.linaro.org
> https://lists.linaro.org/mailman/listinfo/linaro-uefi
Leif Lindholm April 13, 2017, 2:26 p.m. | #9
Thanks guys, pushed as bfce8b2.

On Thu, Apr 13, 2017 at 09:17:34AM -0500, Daniil Egranov wrote:
> Sure.
> 
> Thanks,
> Daniil
> 
> On 04/13/2017 08:39 AM, Leif Lindholm wrote:
> >Daniil, Alan - can I keep your Tested-by:s?
> >
> >/
> >     Leif
> >
> >On Thu, Apr 13, 2017 at 02:13:24PM +0100, Ard Biesheuvel wrote:
> >>On 13 April 2017 at 14:09, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> >>>On Thu, Apr 13, 2017 at 08:29:01AM +0100, Ard Biesheuvel wrote:
> >>>>On 12 April 2017 at 21:47, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> >>>>>Fix a case where MarvellYukonDriverStart attempted to free a buffer
> >>>>>before it was allocated.
> >>>>>
> >>>>>Fix another case where the function returned without freeing the same
> >>>>>buffer. Also correct the error message for that case, which was copied
> >>>>>verbatim from the preceding if-statement.
> >>>>>
> >>>>>Contributed-under: TianoCore Contribution Agreement 1.0
> >>>>>Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>
> >>>>>---
> >>>>>  Drivers/Net/MarvellYukonDxe/DriverBinding.c | 4 ++--
> >>>>>  1 file changed, 2 insertions(+), 2 deletions(-)
> >>>>>
> >>>>>diff --git a/Drivers/Net/MarvellYukonDxe/DriverBinding.c b/Drivers/Net/MarvellYukonDxe/DriverBinding.c
> >>>>>index 947d738b10..cbe8ad34e4 100644
> >>>>>--- a/Drivers/Net/MarvellYukonDxe/DriverBinding.c
> >>>>>+++ b/Drivers/Net/MarvellYukonDxe/DriverBinding.c
> >>>>>@@ -127,7 +127,6 @@ MarvellYukonDriverStart (
> >>>>>
> >>>>>    if (EFI_ERROR (Status)) {
> >>>>>      DEBUG ((EFI_D_ERROR, "Marvell Yukon: OpenProtocol: EFI_PCI_IO_PROTOCOL ERROR Status = %r\n", Status));
> >>>>>-    gBS->FreePool (YukonDriver);
> >>>>>      return Status;
> >>>>>    }
> >>>>>
> >>>>>@@ -156,7 +155,8 @@ MarvellYukonDriverStart (
> >>>>>      }
> >>>>>
> >>>>>      if (ScData->msk_if[Port] == NULL) {
> >>>>>-      DEBUG ((DEBUG_ERROR, "Marvell Yukon: AllocatePool() failed with Status = %r\n", EFI_BAD_BUFFER_SIZE));
> >>>>>+      DEBUG ((DEBUG_ERROR, "Marvell Yukon: invalid buffer size\n"));
> >>>>>+      gBS->FreePool (YukonDriver);
> >>>>>        return EFI_BAD_BUFFER_SIZE;
> >>>>>      }
> >>>>>
> >>>>Wouldn't it make much more sense to move this test above the AllocatePool() ?
> >>>You're not wrong.
> >>>
> >>>How about this?:
> >>>
> >>> From 977ad2b9b4ae02c2e1995279bd49c02f86ccd10a Mon Sep 17 00:00:00 2001
> >>>From: Leif Lindholm <leif.lindholm@linaro.org>
> >>>Date: Wed, 12 Apr 2017 15:23:47 +0100
> >>>Subject: [PATCH] Drivers: MarvellYukonDxe driver binding fixes
> >>>
> >>>Fix a case where MarvellYukonDriverStart attempted to free a buffer
> >>>before it was allocated.
> >>>
> >>>Move another error case which wasn't freeing the same buffer before
> >>>returning. Also correct the error message for that case, which was
> >>>copied verbatim from the preceding if-statement.
> >>>
> >>>Contributed-under: TianoCore Contribution Agreement 1.0
> >>>Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>
> >>>---
> >>>  Drivers/Net/MarvellYukonDxe/DriverBinding.c | 10 ++++------
> >>>  1 file changed, 4 insertions(+), 6 deletions(-)
> >>>
> >>>diff --git a/Drivers/Net/MarvellYukonDxe/DriverBinding.c b/Drivers/Net/MarvellYukonDxe/DriverBinding.c
> >>>index 947d738..0abc94e 100644
> >>>--- a/Drivers/Net/MarvellYukonDxe/DriverBinding.c
> >>>+++ b/Drivers/Net/MarvellYukonDxe/DriverBinding.c
> >>>@@ -127,7 +127,6 @@ MarvellYukonDriverStart (
> >>>
> >>>    if (EFI_ERROR (Status)) {
> >>>      DEBUG ((EFI_D_ERROR, "Marvell Yukon: OpenProtocol: EFI_PCI_IO_PROTOCOL ERROR Status = %r\n", Status));
> >>>-    gBS->FreePool (YukonDriver);
> >>>      return Status;
> >>>    }
> >>>
> >>>@@ -146,6 +145,10 @@ MarvellYukonDriverStart (
> >>>    }
> >>>
> >>>    for (Port = 0; Port < ScData->msk_num_port; Port++) {
> >>>+    if (ScData->msk_if[Port] == NULL) {
> >>>+      DEBUG ((DEBUG_ERROR, "Marvell Yukon: invalid buffer size\n"));
> >>>+      return EFI_BAD_BUFFER_SIZE;
> >>>+    }
> >>>
> >>>      Status = gBS->AllocatePool (EfiBootServicesData,
> >>>                                  sizeof (YUKON_DRIVER),
> >>>@@ -155,11 +158,6 @@ MarvellYukonDriverStart (
> >>>        return Status;
> >>>      }
> >>>
> >>>-    if (ScData->msk_if[Port] == NULL) {
> >>>-      DEBUG ((DEBUG_ERROR, "Marvell Yukon: AllocatePool() failed with Status = %r\n", EFI_BAD_BUFFER_SIZE));
> >>>-      return EFI_BAD_BUFFER_SIZE;
> >>>-    }
> >>>-
> >>>      gBS->SetMem (YukonDriver, sizeof (YUKON_DRIVER), 0);
> >>>      EfiInitializeLock (&YukonDriver->Lock, TPL_NOTIFY);
> >>>
> >>Works for me
> >>
> >>Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> >_______________________________________________
> >Linaro-uefi mailing list
> >Linaro-uefi@lists.linaro.org
> >https://lists.linaro.org/mailman/listinfo/linaro-uefi
>

Patch hide | download patch | download mbox

diff --git a/Drivers/Net/MarvellYukonDxe/DriverBinding.c b/Drivers/Net/MarvellYukonDxe/DriverBinding.c
index 947d738b10..cbe8ad34e4 100644
--- a/Drivers/Net/MarvellYukonDxe/DriverBinding.c
+++ b/Drivers/Net/MarvellYukonDxe/DriverBinding.c
@@ -127,7 +127,6 @@  MarvellYukonDriverStart (
 
   if (EFI_ERROR (Status)) {
     DEBUG ((EFI_D_ERROR, "Marvell Yukon: OpenProtocol: EFI_PCI_IO_PROTOCOL ERROR Status = %r\n", Status));
-    gBS->FreePool (YukonDriver);
     return Status;
   }
 
@@ -156,7 +155,8 @@  MarvellYukonDriverStart (
     }
 
     if (ScData->msk_if[Port] == NULL) {
-      DEBUG ((DEBUG_ERROR, "Marvell Yukon: AllocatePool() failed with Status = %r\n", EFI_BAD_BUFFER_SIZE));
+      DEBUG ((DEBUG_ERROR, "Marvell Yukon: invalid buffer size\n"));
+      gBS->FreePool (YukonDriver);
       return EFI_BAD_BUFFER_SIZE;
     }