[PATCH] ahci: zero-initialize port struct

[PATCH] ahci: zero-initialize port struct

[Gerd Hoffmann]

Specifically port->driver.lchs needs clearing, otherwise seabios will
try interpret whatever random crap happens to be there as disk geometry,
which may or may not break boot depending on how lucky you are.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 src/hw/ahci.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/hw/ahci.c b/src/hw/ahci.c
index 97a072a1ca81..d45b4307ec68 100644
--- a/src/hw/ahci.c
+++ b/src/hw/ahci.c
@@ -345,6 +345,7 @@ ahci_port_alloc(struct ahci_ctrl_s *ctrl, u32 pnr)
         warn_noalloc();
         return NULL;
     }
+    memset(port, 0, sizeof(*port));
     port->pnr = pnr;
     port->ctrl = ctrl;
     port->list = memalign_tmp(1024, 1024);
-- 
2.18.1

Re: [PATCH] ahci: zero-initialize port struct

[Laszlo Ersek]

On 11/13/19 10:18, Gerd Hoffmann wrote:
> Specifically port->driver.lchs needs clearing, otherwise seabios will

s/driver/drive/

> try interpret whatever random crap happens to be there as disk geometry,
> which may or may not break boot depending on how lucky you are.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  src/hw/ahci.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/src/hw/ahci.c b/src/hw/ahci.c
> index 97a072a1ca81..d45b4307ec68 100644
> --- a/src/hw/ahci.c
> +++ b/src/hw/ahci.c
> @@ -345,6 +345,7 @@ ahci_port_alloc(struct ahci_ctrl_s *ctrl, u32 pnr)
>          warn_noalloc();
>          return NULL;
>      }
> +    memset(port, 0, sizeof(*port));
>      port->pnr = pnr;
>      port->ctrl = ctrl;
>      port->list = memalign_tmp(1024, 1024);
> 

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Re: [SeaBIOS] Re: [PATCH] ahci: zero-initialize port struct

[Gerd Hoffmann]

On Wed, Nov 13, 2019 at 10:35:03AM +0100, Laszlo Ersek wrote:
> On 11/13/19 10:18, Gerd Hoffmann wrote:
> > Specifically port->driver.lchs needs clearing, otherwise seabios will
> 
> s/driver/drive/

> Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Typo fixed & pushed.

thanks,
  Gerd

Re: [SeaBIOS] Re: [PATCH] ahci: zero-initialize port struct

[Sam Eiderman]

Hi,

Does this fix a bug that actually happened?

I just noticed that in my lchs patches I assumed that lchs struct is
zeroed out in all devices (not only ahci):

9caa19be0e53 (geometry: Apply LCHS values for boot devices)

Seems like this is not the case but why only ahci is affected?

The list of devices is at least:

        * ata
        * ahci
        * scsi
            * esp
            * lsi
            * megasas
            * mpt
            * pvscsi
            * virtio
        * virtio-blk

As specified in the commit message.

Also Gerd it seems that my lchs patches were not committed in the
latest submitted version (v4)!!!
The ABI of the fw config key is completely broken.


On Wed, Nov 13, 2019 at 4:02 PM Gerd Hoffmann <kraxel@redhat.com> wrote:
>
> On Wed, Nov 13, 2019 at 10:35:03AM +0100, Laszlo Ersek wrote:
> > On 11/13/19 10:18, Gerd Hoffmann wrote:
> > > Specifically port->driver.lchs needs clearing, otherwise seabios will
> >
> > s/driver/drive/
>
> > Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>
> Typo fixed & pushed.
>
> thanks,
>   Gerd
> _______________________________________________
> SeaBIOS mailing list -- seabios@seabios.org
> To unsubscribe send an email to seabios-leave@seabios.org

Re: [SeaBIOS] Re: [PATCH] ahci: zero-initialize port struct

[Sam Eiderman]

We should make sure drive.lchs is zeroed out for the following devices:

git grep "drive_s drive"

src/hw/ata.h:    struct drive_s drive;
src/hw/esp-scsi.c:    struct drive_s drive;
src/hw/lsi-scsi.c:    struct drive_s drive;
src/hw/megasas.c:    struct drive_s drive;
src/hw/mpt-scsi.c:    struct drive_s drive;
src/hw/nvme-int.h:    struct drive_s drive;
src/hw/pvscsi.c:    struct drive_s drive;
src/hw/sdcard.c:    struct drive_s drive;
src/hw/usb-msc.c:    struct drive_s drive;
src/hw/usb-uas.c:    struct drive_s drive;
src/hw/virtio-blk.c:    struct drive_s drive;
src/hw/virtio-scsi.c:    struct drive_s drive;

Sam


On Wed, Nov 13, 2019 at 5:03 PM Sam Eiderman <sameid@google.com> wrote:
>
> Hi,
>
> Does this fix a bug that actually happened?
>
> I just noticed that in my lchs patches I assumed that lchs struct is
> zeroed out in all devices (not only ahci):
>
> 9caa19be0e53 (geometry: Apply LCHS values for boot devices)
>
> Seems like this is not the case but why only ahci is affected?
>
> The list of devices is at least:
>
>         * ata
>         * ahci
>         * scsi
>             * esp
>             * lsi
>             * megasas
>             * mpt
>             * pvscsi
>             * virtio
>         * virtio-blk
>
> As specified in the commit message.
>
> Also Gerd it seems that my lchs patches were not committed in the
> latest submitted version (v4)!!!
> The ABI of the fw config key is completely broken.
>
>
> On Wed, Nov 13, 2019 at 4:02 PM Gerd Hoffmann <kraxel@redhat.com> wrote:
> >
> > On Wed, Nov 13, 2019 at 10:35:03AM +0100, Laszlo Ersek wrote:
> > > On 11/13/19 10:18, Gerd Hoffmann wrote:
> > > > Specifically port->driver.lchs needs clearing, otherwise seabios will
> > >
> > > s/driver/drive/
> >
> > > Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> >
> > Typo fixed & pushed.
> >
> > thanks,
> >   Gerd
> > _______________________________________________
> > SeaBIOS mailing list -- seabios@seabios.org
> > To unsubscribe send an email to seabios-leave@seabios.org

Re: [SeaBIOS] Re: [PATCH] ahci: zero-initialize port struct

[Sam Eiderman]

Sorry the correct list (which includes ahci) is:

git grep "drive_s * drive"

src/hw/ahci.h:    struct drive_s     drive;
src/hw/ata.h:    struct drive_s drive;
src/hw/esp-scsi.c:    struct drive_s drive;
src/hw/lsi-scsi.c:    struct drive_s drive;
src/hw/megasas.c:    struct drive_s drive;
src/hw/mpt-scsi.c:    struct drive_s drive;
src/hw/nvme-int.h:    struct drive_s drive;
src/hw/pvscsi.c:    struct drive_s drive;
src/hw/sdcard.c:    struct drive_s drive;
src/hw/usb-msc.c:    struct drive_s drive;
src/hw/usb-uas.c:    struct drive_s drive;
src/hw/virtio-blk.c:    struct drive_s drive;
src/hw/virtio-scsi.c:    struct drive_s drive;

Went over this list now, seems only ahci_port_alloc was missing
memset(0) so after this patch all of the devices that contain lchs are
covered.

But we still need to revert my lchs patches and reapply them with the
newest version (v4)

Sam

On Wed, Nov 13, 2019 at 5:18 PM Sam Eiderman <sameid@google.com> wrote:
>
> We should make sure drive.lchs is zeroed out for the following devices:
>
> git grep "drive_s drive"
>
> src/hw/ata.h:    struct drive_s drive;
> src/hw/esp-scsi.c:    struct drive_s drive;
> src/hw/lsi-scsi.c:    struct drive_s drive;
> src/hw/megasas.c:    struct drive_s drive;
> src/hw/mpt-scsi.c:    struct drive_s drive;
> src/hw/nvme-int.h:    struct drive_s drive;
> src/hw/pvscsi.c:    struct drive_s drive;
> src/hw/sdcard.c:    struct drive_s drive;
> src/hw/usb-msc.c:    struct drive_s drive;
> src/hw/usb-uas.c:    struct drive_s drive;
> src/hw/virtio-blk.c:    struct drive_s drive;
> src/hw/virtio-scsi.c:    struct drive_s drive;
>
> Sam
>
>
> On Wed, Nov 13, 2019 at 5:03 PM Sam Eiderman <sameid@google.com> wrote:
> >
> > Hi,
> >
> > Does this fix a bug that actually happened?
> >
> > I just noticed that in my lchs patches I assumed that lchs struct is
> > zeroed out in all devices (not only ahci):
> >
> > 9caa19be0e53 (geometry: Apply LCHS values for boot devices)
> >
> > Seems like this is not the case but why only ahci is affected?
> >
> > The list of devices is at least:
> >
> >         * ata
> >         * ahci
> >         * scsi
> >             * esp
> >             * lsi
> >             * megasas
> >             * mpt
> >             * pvscsi
> >             * virtio
> >         * virtio-blk
> >
> > As specified in the commit message.
> >
> > Also Gerd it seems that my lchs patches were not committed in the
> > latest submitted version (v4)!!!
> > The ABI of the fw config key is completely broken.
> >
> >
> > On Wed, Nov 13, 2019 at 4:02 PM Gerd Hoffmann <kraxel@redhat.com> wrote:
> > >
> > > On Wed, Nov 13, 2019 at 10:35:03AM +0100, Laszlo Ersek wrote:
> > > > On 11/13/19 10:18, Gerd Hoffmann wrote:
> > > > > Specifically port->driver.lchs needs clearing, otherwise seabios will
> > > >
> > > > s/driver/drive/
> > >
> > > > Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> > >
> > > Typo fixed & pushed.
> > >
> > > thanks,
> > >   Gerd
> > > _______________________________________________
> > > SeaBIOS mailing list -- seabios@seabios.org
> > > To unsubscribe send an email to seabios-leave@seabios.org

Re: [SeaBIOS] Re: [PATCH] ahci: zero-initialize port struct

[Philippe Mathieu-Daudé]

Hi Sam,

On 11/13/19 4:03 PM, Sam Eiderman wrote:
> Hi,
> 
> Does this fix a bug that actually happened?
> 
> I just noticed that in my lchs patches I assumed that lchs struct is
> zeroed out in all devices (not only ahci):
> 
> 9caa19be0e53 (geometry: Apply LCHS values for boot devices)
> 
> Seems like this is not the case but why only ahci is affected?
> 
> The list of devices is at least:
> 
>          * ata
>          * ahci
>          * scsi
>              * esp
>              * lsi
>              * megasas
>              * mpt
>              * pvscsi
>              * virtio
>          * virtio-blk
> 
> As specified in the commit message.
> 
> Also Gerd it seems that my lchs patches were not committed in the
> latest submitted version (v4)!!!
> The ABI of the fw config key is completely broken.

What do you mean? Can you be more specific?

Re: [SeaBIOS] Re: [PATCH] ahci: zero-initialize port struct

[Sam Eiderman]

Sure,

There are two issues here.

The first issue is that my commits which applied to seabios master:

* 9caa19b - geometry: Apply LCHS values for boot devices
* cb56f61 - config: Add toggle for bootdevice information
* ad29109 - geometry: Add boot_lchs_find_*() utility functions
* b3d2120 - boot: Reorder functions in boot.c
* 7c66a43 - geometry: Read LCHS from fw_cfg

Are not from the latest version which was submitted to the mailing list (v4)
* fw_cfg key name has changed
* The value and of the key has changed from binary (v1) to textual (v4)
* Other fixes and variable name changes.

So these commits need to be reverted and reapplied with the latest version (v4)

The second issue is that my commits, (in v4 too) will require this fix
that Gerd added ([PATCH] ahci: zero-initialize port struct) since they
change how SeaBIOS uses lchs.

Previously, before any of my commits, drive.lchs could contain "random
crap" since it was always set before being used in
setup_translation().

After my patches, get_translation() invokes overriden_lchs_supplied()
which checks: "return drive->lchs.cylinder || drive->lchs.head ||
drive->lchs.sector;"
So there is an assumption that "drive->lchs" is zeroed when lchs is
not supplied for the host.

This was true for all devices using "drive->lchs" (all were memset to
0) except ahci.
(I used 'git grep "drive_s * drive"' to find them all).

So Gerd fix is indeed needed and then all devices are covered
(drive->lchs is memset to 0).

Now only the first issue remains...

Sam

On Wed, Nov 13, 2019 at 6:12 PM Philippe Mathieu-Daudé
<philmd@redhat.com> wrote:
>
> Hi Sam,
>
> On 11/13/19 4:03 PM, Sam Eiderman wrote:
> > Hi,
> >
> > Does this fix a bug that actually happened?
> >
> > I just noticed that in my lchs patches I assumed that lchs struct is
> > zeroed out in all devices (not only ahci):
> >
> > 9caa19be0e53 (geometry: Apply LCHS values for boot devices)
> >
> > Seems like this is not the case but why only ahci is affected?
> >
> > The list of devices is at least:
> >
> >          * ata
> >          * ahci
> >          * scsi
> >              * esp
> >              * lsi
> >              * megasas
> >              * mpt
> >              * pvscsi
> >              * virtio
> >          * virtio-blk
> >
> > As specified in the commit message.
> >
> > Also Gerd it seems that my lchs patches were not committed in the
> > latest submitted version (v4)!!!
> > The ABI of the fw config key is completely broken.
>
> What do you mean? Can you be more specific?
>

Re: [SeaBIOS] Re: [PATCH] ahci: zero-initialize port struct

[Sam Eiderman]

Links to latest commits from archive.
You can see all changes in the cover letter.

[SeaBIOS] [PATCH v4 0/5] Add Qemu to SeaBIOS LCHS interface
https://mail.coreboot.org/hyperkitty/list/seabios@seabios.org/message/VLNFBEERTWLEUO6LM5BYLBNVIFCTP46M/
[SeaBIOS] [PATCH v4 1/5] geometry: Read LCHS from fw_cfg
https://mail.coreboot.org/hyperkitty/list/seabios@seabios.org/message/B3IPD3HH4UPDYJWFE4KX3HXUCNW5GPEW/
[SeaBIOS] [PATCH v4 2/5] boot: Reorder functions in boot.c
https://mail.coreboot.org/hyperkitty/list/seabios@seabios.org/message/YDVU3WIGOSKZ2RQSMR5UVQNZ66K4IG65/
[SeaBIOS] [PATCH v4 3/5] boot: Build ata and scsi paths in function
https://mail.coreboot.org/hyperkitty/list/seabios@seabios.org/message/RY33DRZZ3UK3UMQ3Q6BY2KUCHRRW4MRK/
[SeaBIOS] [PATCH v4 4/5] geometry: Add boot_lchs_find_*() utility functions
https://mail.coreboot.org/hyperkitty/list/seabios@seabios.org/message/DAJOULWFK24DX4DY3VS6WWOOQNWW3GSG/
[SeaBIOS] [PATCH v4 5/5] geometry: Apply LCHS values for boot devices
https://mail.coreboot.org/hyperkitty/list/seabios@seabios.org/message/UUCTPPJ4PTS5CUTCFLOH3YOEXGC6HQ4T/

Sam

On Wed, Nov 13, 2019 at 6:35 PM Sam Eiderman <sameid@google.com> wrote:
>
> Sure,
>
> There are two issues here.
>
> The first issue is that my commits which applied to seabios master:
>
> * 9caa19b - geometry: Apply LCHS values for boot devices
> * cb56f61 - config: Add toggle for bootdevice information
> * ad29109 - geometry: Add boot_lchs_find_*() utility functions
> * b3d2120 - boot: Reorder functions in boot.c
> * 7c66a43 - geometry: Read LCHS from fw_cfg
>
> Are not from the latest version which was submitted to the mailing list (v4)
> * fw_cfg key name has changed
> * The value and of the key has changed from binary (v1) to textual (v4)
> * Other fixes and variable name changes.
>
> So these commits need to be reverted and reapplied with the latest version (v4)
>
> The second issue is that my commits, (in v4 too) will require this fix
> that Gerd added ([PATCH] ahci: zero-initialize port struct) since they
> change how SeaBIOS uses lchs.
>
> Previously, before any of my commits, drive.lchs could contain "random
> crap" since it was always set before being used in
> setup_translation().
>
> After my patches, get_translation() invokes overriden_lchs_supplied()
> which checks: "return drive->lchs.cylinder || drive->lchs.head ||
> drive->lchs.sector;"
> So there is an assumption that "drive->lchs" is zeroed when lchs is
> not supplied for the host.
>
> This was true for all devices using "drive->lchs" (all were memset to
> 0) except ahci.
> (I used 'git grep "drive_s * drive"' to find them all).
>
> So Gerd fix is indeed needed and then all devices are covered
> (drive->lchs is memset to 0).
>
> Now only the first issue remains...
>
> Sam
>
> On Wed, Nov 13, 2019 at 6:12 PM Philippe Mathieu-Daudé
> <philmd@redhat.com> wrote:
> >
> > Hi Sam,
> >
> > On 11/13/19 4:03 PM, Sam Eiderman wrote:
> > > Hi,
> > >
> > > Does this fix a bug that actually happened?
> > >
> > > I just noticed that in my lchs patches I assumed that lchs struct is
> > > zeroed out in all devices (not only ahci):
> > >
> > > 9caa19be0e53 (geometry: Apply LCHS values for boot devices)
> > >
> > > Seems like this is not the case but why only ahci is affected?
> > >
> > > The list of devices is at least:
> > >
> > >          * ata
> > >          * ahci
> > >          * scsi
> > >              * esp
> > >              * lsi
> > >              * megasas
> > >              * mpt
> > >              * pvscsi
> > >              * virtio
> > >          * virtio-blk
> > >
> > > As specified in the commit message.
> > >
> > > Also Gerd it seems that my lchs patches were not committed in the
> > > latest submitted version (v4)!!!
> > > The ABI of the fw config key is completely broken.
> >
> > What do you mean? Can you be more specific?
> >

Re: [SeaBIOS] Re: [PATCH] ahci: zero-initialize port struct

[Gerd Hoffmann]

On Wed, Nov 13, 2019 at 05:03:58PM +0200, Sam Eiderman wrote:
> Hi,
> 
> Does this fix a bug that actually happened?

Yes, "make check-qtest" may fail.  It's kind of random though.

> I just noticed that in my lchs patches I assumed that lchs struct is
> zeroed out in all devices (not only ahci):

ahci was the only one not zeroing out the struct (yes, I've reviewed
them all).

> Also Gerd it seems that my lchs patches were not committed in the
> latest submitted version (v4)!!!

Whoops.  Can you sent a patch seabios/master ... v4 please?

IIRC there didn't change much, mostly the parser function, so that
should be alot less churn than a full revert + v4 reapply.

thanks,
  Gerd

Re: [SeaBIOS] Re: [PATCH] ahci: zero-initialize port struct

[Sam Eiderman]

Do you want a single commit with the changes?

I am afraid it will be slightly unreadable when looking at file histories.
The only commit that didn't change was:
[SeaBIOS] [PATCH v4 2/5] boot: Reorder functions in boot.c

Sam

On Thu, Nov 14, 2019 at 9:20 AM Gerd Hoffmann <kraxel@redhat.com> wrote:
>
> On Wed, Nov 13, 2019 at 05:03:58PM +0200, Sam Eiderman wrote:
> > Hi,
> >
> > Does this fix a bug that actually happened?
>
> Yes, "make check-qtest" may fail.  It's kind of random though.
>
> > I just noticed that in my lchs patches I assumed that lchs struct is
> > zeroed out in all devices (not only ahci):
>
> ahci was the only one not zeroing out the struct (yes, I've reviewed
> them all).
>
> > Also Gerd it seems that my lchs patches were not committed in the
> > latest submitted version (v4)!!!
>
> Whoops.  Can you sent a patch seabios/master ... v4 please?
>
> IIRC there didn't change much, mostly the parser function, so that
> should be alot less churn than a full revert + v4 reapply.
>
> thanks,
>   Gerd
>

Re: [SeaBIOS] Re: [PATCH] ahci: zero-initialize port struct

[Gerd Hoffmann]

On Thu, Nov 14, 2019 at 11:08:59AM +0200, Sam Eiderman wrote:
> Do you want a single commit with the changes?

That is the idea, unless it is too messy.  I don't have v4 any more,
looks like I've deleted v4 instead of v3 while cleaning up my mail
folders, so I can't easily check.  Do you have v3 and v4 as git
branches somewhere?

> I am afraid it will be slightly unreadable when looking at file histories.
> The only commit that didn't change was:
> [SeaBIOS] [PATCH v4 2/5] boot: Reorder functions in boot.c

Hmm, looks like there have been more changes than I remember.
Maybe it's better to revert and re-apply the changed patches
then.

cheers,
  Gerd

Re: [SeaBIOS] Re: [PATCH] ahci: zero-initialize port struct

[Gerd Hoffmann]

  Hi,

> > I am afraid it will be slightly unreadable when looking at file histories.
> > The only commit that didn't change was:
> > [SeaBIOS] [PATCH v4 2/5] boot: Reorder functions in boot.c
> 
> Hmm, looks like there have been more changes than I remember.

Yep, v3..v4 diff is pretty big, so revert + apply v4 looks more
reasonable.  Can you have a look and double-check?

https://git.kraxel.org/cgit/seabios/log/?h=lchs-fixup

thanks,
  Gerd

Re: [SeaBIOS] Re: [PATCH] ahci: zero-initialize port struct

[Sam Eiderman]

Looks great

Sam

On Fri, Nov 15, 2019 at 1:35 PM Gerd Hoffmann <kraxel@redhat.com> wrote:
>
>   Hi,
>
> > > I am afraid it will be slightly unreadable when looking at file histories.
> > > The only commit that didn't change was:
> > > [SeaBIOS] [PATCH v4 2/5] boot: Reorder functions in boot.c
> >
> > Hmm, looks like there have been more changes than I remember.
>
> Yep, v3..v4 diff is pretty big, so revert + apply v4 looks more
> reasonable.  Can you have a look and double-check?
>
> https://git.kraxel.org/cgit/seabios/log/?h=lchs-fixup
>
> thanks,
>   Gerd
>