[PATCH] osdep/linux: convert partition start to disk sector length.

[PATCH] osdep/linux: convert partition start to disk sector length.

[Mihai Moldovan]

When reading data off a disk, sector values are based on the disk sector
length.

Within grub_util_fd_open_device(), the start of the partition was taken
directly from grub's partition information structure, which uses the
internal sector length (currently 512b), but never transformed to the
disk's sector length.

Subsequent calculations were all wrong for devices that have a diverging
sector length and the functions eventually skipped to the wrong stream
location, reading invalid data.
---
 grub-core/osdep/linux/hostdisk.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/grub-core/osdep/linux/hostdisk.c b/grub-core/osdep/linux/hostdisk.c
index 06179fca7..8b92f8528 100644
--- a/grub-core/osdep/linux/hostdisk.c
+++ b/grub-core/osdep/linux/hostdisk.c
@@ -374,7 +374,8 @@ grub_util_fd_open_device (const grub_disk_t disk, grub_disk_addr_t sector, int f
     char dev[PATH_MAX];
     grub_disk_addr_t part_start = 0;
 
-    part_start = grub_partition_get_start (disk->partition);
+    part_start = grub_partition_get_start (disk->partition)
+                 >> (disk->log_sector_size - GRUB_DISK_SECTOR_BITS);
 
     strncpy (dev, grub_util_biosdisk_get_osdev (disk), sizeof (dev) - 1);
     dev[sizeof(dev) - 1] = '\0';
-- 
2.15.1

Re: [PATCH] osdep/linux: convert partition start to disk sector length.

[Daniel Kiper]

On Mon, Jul 23, 2018 at 08:52:50AM +0200, Mihai Moldovan wrote:
> When reading data off a disk, sector values are based on the disk sector
> length.
>
> Within grub_util_fd_open_device(), the start of the partition was taken
> directly from grub's partition information structure, which uses the
> internal sector length (currently 512b), but never transformed to the
> disk's sector length.
>
> Subsequent calculations were all wrong for devices that have a diverging
> sector length and the functions eventually skipped to the wrong stream
> location, reading invalid data.

Lack of SOB... May I ask you to add it? Otherwise LGTM.

Daniel

[PATCH v2] osdep/linux: convert partition start to disk sector length.

[Mihai Moldovan]

When reading data off a disk, sector values are based on the disk sector
length.

Within grub_util_fd_open_device(), the start of the partition was taken
directly from grub's partition information structure, which uses the
internal sector length (currently 512b), but never transformed to the
disk's sector length.

Subsequent calculations were all wrong for devices that have a diverging
sector length and the functions eventually skipped to the wrong stream
location, reading invalid data.

Signed-off-by: Mihai Moldovan <ionic@ionic.de>
---
 grub-core/osdep/linux/hostdisk.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/grub-core/osdep/linux/hostdisk.c b/grub-core/osdep/linux/hostdisk.c
index 06179fca7..8b92f8528 100644
--- a/grub-core/osdep/linux/hostdisk.c
+++ b/grub-core/osdep/linux/hostdisk.c
@@ -374,7 +374,8 @@ grub_util_fd_open_device (const grub_disk_t disk, grub_disk_addr_t sector, int f
     char dev[PATH_MAX];
     grub_disk_addr_t part_start = 0;
 
-    part_start = grub_partition_get_start (disk->partition);
+    part_start = grub_partition_get_start (disk->partition)
+                 >> (disk->log_sector_size - GRUB_DISK_SECTOR_BITS);
 
     strncpy (dev, grub_util_biosdisk_get_osdev (disk), sizeof (dev) - 1);
     dev[sizeof(dev) - 1] = '\0';
-- 
2.15.1

Re: [PATCH v2] osdep/linux: convert partition start to disk sector length.

[Daniel Kiper]

On Thu, Sep 06, 2018 at 04:16:39PM +0200, Mihai Moldovan wrote:
> When reading data off a disk, sector values are based on the disk sector
> length.
>
> Within grub_util_fd_open_device(), the start of the partition was taken
> directly from grub's partition information structure, which uses the
> internal sector length (currently 512b), but never transformed to the
> disk's sector length.
>
> Subsequent calculations were all wrong for devices that have a diverging
> sector length and the functions eventually skipped to the wrong stream
> location, reading invalid data.
>
> Signed-off-by: Mihai Moldovan <ionic@ionic.de>

Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>

Daniel

Re: [PATCH] osdep/linux: convert partition start to disk sector length.

[Vladimir 'phcoder' Serbinenko]

[-- Attachment #1: Type: text/plain, Size: 1644 bytes --]

Grub partition get start returns in 512B blocks. Can I have more details
like which partmap you use?

On Mon, 23 Jul 2018, 09:54 Mihai Moldovan, <ionic@ionic.de> wrote:

> When reading data off a disk, sector values are based on the disk sector
> length.
>
> Within grub_util_fd_open_device(), the start of the partition was taken
> directly from grub's partition information structure, which uses the
> internal sector length (currently 512b), but never transformed to the
> disk's sector length.
>
> Subsequent calculations were all wrong for devices that have a diverging
> sector length and the functions eventually skipped to the wrong stream
> location, reading invalid data.
> ---
>  grub-core/osdep/linux/hostdisk.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/grub-core/osdep/linux/hostdisk.c
> b/grub-core/osdep/linux/hostdisk.c
> index 06179fca7..8b92f8528 100644
> --- a/grub-core/osdep/linux/hostdisk.c
> +++ b/grub-core/osdep/linux/hostdisk.c
> @@ -374,7 +374,8 @@ grub_util_fd_open_device (const grub_disk_t disk,
> grub_disk_addr_t sector, int f
>      char dev[PATH_MAX];
>      grub_disk_addr_t part_start = 0;
>
> -    part_start = grub_partition_get_start (disk->partition);
> +    part_start = grub_partition_get_start (disk->partition)
> +                 >> (disk->log_sector_size - GRUB_DISK_SECTOR_BITS);
>
>      strncpy (dev, grub_util_biosdisk_get_osdev (disk), sizeof (dev) - 1);
>      dev[sizeof(dev) - 1] = '\0';
> --
> 2.15.1
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
>

[-- Attachment #2: Type: text/html, Size: 2187 bytes --]

Re: [PATCH] osdep/linux: convert partition start to disk sector length.

[Mihai Moldovan]

[-- Attachment #1.1: Type: text/plain, Size: 502 bytes --]

* On 09/06/2018 07:36 PM, Vladimir 'phcoder' Serbinenko wrote:
> Grub partition get start returns in 512B blocks. Can I have more details like
> which partmap you use?

Yes, but this is a low-level function that expects sector sizes in HW sector
addressing mode.


Note that the whole sector conversion magic was already done by grub_disk_read()
and friends which then calls the fd_open() function.

Encountered the issue with a GPT-partitioned, 4096-bytes-sector-size disk.



Mihai


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 898 bytes --]

Re: [PATCH] osdep/linux: convert partition start to disk sector length.

[Daniel Kiper]

On Thu, Sep 06, 2018 at 08:02:02PM +0200, Mihai Moldovan wrote:
> * On 09/06/2018 07:36 PM, Vladimir 'phcoder' Serbinenko wrote:
> > Grub partition get start returns in 512B blocks. Can I have more details like
> > which partmap you use?
>
> Yes, but this is a low-level function that expects sector sizes in HW sector
> addressing mode.
>
>
> Note that the whole sector conversion magic was already done by grub_disk_read()
> and friends which then calls the fd_open() function.
>
> Encountered the issue with a GPT-partitioned, 4096-bytes-sector-size disk.

If there are no more questions or objections I will push this patch next week.

Daniel

Re: [PATCH] osdep/linux: convert partition start to disk sector length.

[Daniel Kiper]

On Wed, Sep 19, 2018 at 03:58:56PM +0200, Daniel Kiper wrote:
> On Thu, Sep 06, 2018 at 08:02:02PM +0200, Mihai Moldovan wrote:
> > * On 09/06/2018 07:36 PM, Vladimir 'phcoder' Serbinenko wrote:
> > > Grub partition get start returns in 512B blocks. Can I have more details like
> > > which partmap you use?
> >
> > Yes, but this is a low-level function that expects sector sizes in HW sector
> > addressing mode.
> >
> >
> > Note that the whole sector conversion magic was already done by grub_disk_read()
> > and friends which then calls the fd_open() function.
> >
> > Encountered the issue with a GPT-partitioned, 4096-bytes-sector-size disk.
>
> If there are no more questions or objections I will push this patch next week.

Err... Of course v2.

Daniel