[PATCH] Fix bus error when accessing MBR partition records

[PATCH] Fix bus error when accessing MBR partition records

[James Clarke]

Since the MBR layout only has partition records as 2-byte aligned, the 32-bit
fields in them are not aligned. Thus, they cannot be accessed on some
architectures (such as SPARC) by using a "struct MBR_part_record *" pointer,
as the compiler can assume that the pointer is properly aligned. Instead, the
records must be accessed by going through the MBR struct itself every time.

Signed-off-by: James Clarke <jrtc27@jrtc27.com>
---
 super-mbr.c |  6 ++++++
 util.c      | 14 +++++++-------
 2 files changed, 13 insertions(+), 7 deletions(-)

diff --git a/super-mbr.c b/super-mbr.c
index 62b3f03..303dde4 100644
--- a/super-mbr.c
+++ b/super-mbr.c
@@ -57,6 +57,9 @@ static void examine_mbr(struct supertype *st, char *homehost)
 
 	printf("   MBR Magic : %04x\n", sb->magic);
 	for (i = 0; i < MBR_PARTITIONS; i++)
+		/* Have to make every access through sb rather than using a pointer to
+		 * the partition table (or an entry), since the entries are not
+		 * properly aligned. */
 		if (sb->parts[i].blocks_num)
 			printf("Partition[%d] : %12lu sectors at %12lu (type %02x)\n",
 			       i,
@@ -151,6 +154,9 @@ static void getinfo_mbr(struct supertype *st, struct mdinfo *info, char *map)
 	info->component_size = 0;
 
 	for (i = 0; i < MBR_PARTITIONS ; i++)
+		/* Have to make every access through sb rather than using a pointer to
+		 * the partition table (or an entry), since the entries are not
+		 * properly aligned. */
 		if (sb->parts[i].blocks_num) {
 			unsigned long last =
 				(unsigned long)__le32_to_cpu(sb->parts[i].blocks_num)
diff --git a/util.c b/util.c
index a238a21..08adbd5 100644
--- a/util.c
+++ b/util.c
@@ -1412,7 +1412,6 @@ static int get_gpt_last_partition_end(int fd, unsigned long long *endofpart)
 static int get_last_partition_end(int fd, unsigned long long *endofpart)
 {
 	struct MBR boot_sect;
-	struct MBR_part_record *part;
 	unsigned long long curr_part_end;
 	unsigned part_nr;
 	int retval = 0;
@@ -1429,21 +1428,22 @@ static int get_last_partition_end(int fd, unsigned long long *endofpart)
 	if (boot_sect.magic == MBR_SIGNATURE_MAGIC) {
 		retval = 1;
 		/* found the correct signature */
-		part = boot_sect.parts;
 
 		for (part_nr = 0; part_nr < MBR_PARTITIONS; part_nr++) {
+			/* Have to make every access through boot_sect rather than using a
+			 * pointer to the partition table (or an entry), since the entries
+			 * are not properly aligned. */
+
 			/* check for GPT type */
-			if (part->part_type == MBR_GPT_PARTITION_TYPE) {
+			if (boot_sect.parts[part_nr].part_type == MBR_GPT_PARTITION_TYPE) {
 				retval = get_gpt_last_partition_end(fd, endofpart);
 				break;
 			}
 			/* check the last used lba for the current partition  */
-			curr_part_end = __le32_to_cpu(part->first_sect_lba) +
-				__le32_to_cpu(part->blocks_num);
+			curr_part_end = __le32_to_cpu(boot_sect.parts[part_nr].first_sect_lba) +
+				__le32_to_cpu(boot_sect.parts[part_nr].blocks_num);
 			if (curr_part_end > *endofpart)
 				*endofpart = curr_part_end;
-
-			part++;
 		}
 	} else {
 		/* Unknown partition table */
-- 
2.10.0

Re: [PATCH] Fix bus error when accessing MBR partition records

[NeilBrown]

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

On Thu, Sep 29 2016, James Clarke wrote:

> Since the MBR layout only has partition records as 2-byte aligned, the 32-bit
> fields in them are not aligned. Thus, they cannot be accessed on some
> architectures (such as SPARC) by using a "struct MBR_part_record *" pointer,
> as the compiler can assume that the pointer is properly aligned. Instead, the
> records must be accessed by going through the MBR struct itself every time.

Weird....

Can you see if adding "__attribute__((packed))" to struct
MBR_part_record also fixes the problem?

It seems strange that the compiler lets you take a pointer, but then
doesn't use it correctly.  Maybe it is an inconsistency in the types.

I don't necessarily disagree with your fix, but I'd like to understand
why the current code is wrong.

Thanks,
NeilBrown


>
> Signed-off-by: James Clarke <jrtc27@jrtc27.com>
> ---
>  super-mbr.c |  6 ++++++
>  util.c      | 14 +++++++-------
>  2 files changed, 13 insertions(+), 7 deletions(-)
>
> diff --git a/super-mbr.c b/super-mbr.c
> index 62b3f03..303dde4 100644
> --- a/super-mbr.c
> +++ b/super-mbr.c
> @@ -57,6 +57,9 @@ static void examine_mbr(struct supertype *st, char *homehost)
>  
>  	printf("   MBR Magic : %04x\n", sb->magic);
>  	for (i = 0; i < MBR_PARTITIONS; i++)
> +		/* Have to make every access through sb rather than using a pointer to
> +		 * the partition table (or an entry), since the entries are not
> +		 * properly aligned. */
>  		if (sb->parts[i].blocks_num)
>  			printf("Partition[%d] : %12lu sectors at %12lu (type %02x)\n",
>  			       i,
> @@ -151,6 +154,9 @@ static void getinfo_mbr(struct supertype *st, struct mdinfo *info, char *map)
>  	info->component_size = 0;
>  
>  	for (i = 0; i < MBR_PARTITIONS ; i++)
> +		/* Have to make every access through sb rather than using a pointer to
> +		 * the partition table (or an entry), since the entries are not
> +		 * properly aligned. */
>  		if (sb->parts[i].blocks_num) {
>  			unsigned long last =
>  				(unsigned long)__le32_to_cpu(sb->parts[i].blocks_num)
> diff --git a/util.c b/util.c
> index a238a21..08adbd5 100644
> --- a/util.c
> +++ b/util.c
> @@ -1412,7 +1412,6 @@ static int get_gpt_last_partition_end(int fd, unsigned long long *endofpart)
>  static int get_last_partition_end(int fd, unsigned long long *endofpart)
>  {
>  	struct MBR boot_sect;
> -	struct MBR_part_record *part;
>  	unsigned long long curr_part_end;
>  	unsigned part_nr;
>  	int retval = 0;
> @@ -1429,21 +1428,22 @@ static int get_last_partition_end(int fd, unsigned long long *endofpart)
>  	if (boot_sect.magic == MBR_SIGNATURE_MAGIC) {
>  		retval = 1;
>  		/* found the correct signature */
> -		part = boot_sect.parts;
>  
>  		for (part_nr = 0; part_nr < MBR_PARTITIONS; part_nr++) {
> +			/* Have to make every access through boot_sect rather than using a
> +			 * pointer to the partition table (or an entry), since the entries
> +			 * are not properly aligned. */
> +
>  			/* check for GPT type */
> -			if (part->part_type == MBR_GPT_PARTITION_TYPE) {
> +			if (boot_sect.parts[part_nr].part_type == MBR_GPT_PARTITION_TYPE) {
>  				retval = get_gpt_last_partition_end(fd, endofpart);
>  				break;
>  			}
>  			/* check the last used lba for the current partition  */
> -			curr_part_end = __le32_to_cpu(part->first_sect_lba) +
> -				__le32_to_cpu(part->blocks_num);
> +			curr_part_end = __le32_to_cpu(boot_sect.parts[part_nr].first_sect_lba) +
> +				__le32_to_cpu(boot_sect.parts[part_nr].blocks_num);
>  			if (curr_part_end > *endofpart)
>  				*endofpart = curr_part_end;
> -
> -			part++;
>  		}
>  	} else {
>  		/* Unknown partition table */
> -- 
> 2.10.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-raid" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 800 bytes --]

Re: [PATCH] Fix bus error when accessing MBR partition records

[James Clarke]

Hi Neil,

> On 2 Oct 2016, at 23:32, NeilBrown <neilb@suse.com> wrote:
> 
>> On Thu, Sep 29 2016, James Clarke wrote:
>> 
>> Since the MBR layout only has partition records as 2-byte aligned, the 32-bit
>> fields in them are not aligned. Thus, they cannot be accessed on some
>> architectures (such as SPARC) by using a "struct MBR_part_record *" pointer,
>> as the compiler can assume that the pointer is properly aligned. Instead, the
>> records must be accessed by going through the MBR struct itself every time.
> 
> Weird....
> 
> Can you see if adding "__attribute__((packed))" to struct
> MBR_part_record also fixes the problem?

That also works. When I wrote the patch initially, I wasn’t sure if it was a
"correct" fix, but having looked into it more I *believe* it is conformant. The
alignment of a packed struct is 1-byte, so, while the compiler may know that the
32-bit fields are 8-byte aligned within the struct, the pointer to the struct
need not be aligned, and so the correct conservative code is generated.

> It seems strange that the compiler lets you take a pointer, but then
> doesn't use it correctly.  Maybe it is an inconsistency in the types.

Yes, the type doesn’t include the provenance of the pointer, so in general the
compiler can’t know it came from a packed struct (although in this case not much
static analysis would be needed). See [1] and [2].

> I don't necessarily disagree with your fix, but I'd like to understand
> why the current code is wrong.

Hopefully the links make it clearer.

Regards,
James

[1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=51628
[2] https://llvm.org/bugs/show_bug.cgi?id=22821

Re: [PATCH] Fix bus error when accessing MBR partition records

[NeilBrown]

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

On Mon, Oct 03 2016, James Clarke wrote:

> Hi Neil,
>
>> On 2 Oct 2016, at 23:32, NeilBrown <neilb@suse.com> wrote:
>> 
>>> On Thu, Sep 29 2016, James Clarke wrote:
>>> 
>>> Since the MBR layout only has partition records as 2-byte aligned, the 32-bit
>>> fields in them are not aligned. Thus, they cannot be accessed on some
>>> architectures (such as SPARC) by using a "struct MBR_part_record *" pointer,
>>> as the compiler can assume that the pointer is properly aligned. Instead, the
>>> records must be accessed by going through the MBR struct itself every time.
>> 
>> Weird....
>> 
>> Can you see if adding "__attribute__((packed))" to struct
>> MBR_part_record also fixes the problem?
>
> That also works. When I wrote the patch initially, I wasn’t sure if it was a
> "correct" fix, but having looked into it more I *believe* it is conformant. The
> alignment of a packed struct is 1-byte, so, while the compiler may know that the
> 32-bit fields are 8-byte aligned within the struct, the pointer to the struct
> need not be aligned, and so the correct conservative code is generated.
>
>> It seems strange that the compiler lets you take a pointer, but then
>> doesn't use it correctly.  Maybe it is an inconsistency in the types.
>
> Yes, the type doesn’t include the provenance of the pointer, so in general the
> compiler can’t know it came from a packed struct (although in this case not much
> static analysis would be needed). See [1] and [2].
>
>> I don't necessarily disagree with your fix, but I'd like to understand
>> why the current code is wrong.
>
> Hopefully the links make it clearer.
>
> Regards,
> James
>
> [1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=51628
> [2] https://llvm.org/bugs/show_bug.cgi?id=22821

Thanks.

It looks as though the type change I suggested would work, but probably isn't
the best solution.
Your patch is probably safest, though adding the __attribute__((packed))
as well wouldn't hurt.

I'll leave it for Jes to decide what exactly to apply, but I can offer
a

  Reviewed-by: NeilBrown <neilb@suse.com>

for you patch.

BTW I tried compiling mdadm with clang to see if my clang was new enough
to give a warning (it isn't) but it found a few other things to give
errors about ... I should post patches.

Thanks,
NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 800 bytes --]

Re: [PATCH] Fix bus error when accessing MBR partition records

[Jes Sorensen]

James Clarke <jrtc27@jrtc27.com> writes:
> Since the MBR layout only has partition records as 2-byte aligned, the 32-bit
> fields in them are not aligned. Thus, they cannot be accessed on some
> architectures (such as SPARC) by using a "struct MBR_part_record *" pointer,
> as the compiler can assume that the pointer is properly aligned. Instead, the
> records must be accessed by going through the MBR struct itself every time.
>
> Signed-off-by: James Clarke <jrtc27@jrtc27.com>
> ---
>  super-mbr.c |  6 ++++++
>  util.c      | 14 +++++++-------
>  2 files changed, 13 insertions(+), 7 deletions(-)
>
> diff --git a/super-mbr.c b/super-mbr.c
> index 62b3f03..303dde4 100644
> --- a/super-mbr.c
> +++ b/super-mbr.c
> @@ -57,6 +57,9 @@ static void examine_mbr(struct supertype *st, char *homehost)
>  
>  	printf("   MBR Magic : %04x\n", sb->magic);
>  	for (i = 0; i < MBR_PARTITIONS; i++)
> +		/* Have to make every access through sb rather than using a pointer to
> +		 * the partition table (or an entry), since the entries are not
> +		 * properly aligned. */
>  		if (sb->parts[i].blocks_num)
>  			printf("Partition[%d] : %12lu sectors at %12lu (type %02x)\n",
>  			       i,

Reading through this thread and Neil's comments, I think it's reasonable
to do what you are doing with pointer access. However I also believe
that packed should be applied as Neil suggests.

Second, as code lines per definition are 80 characters wide, please make
sure your patch complies with this. I know some of the older code
violates this, but we shouldn't be adding more code which does so.

If you send me an updated patch I shall be happy to apply it.

Jes

[PATCH v2] Fix bus error when accessing MBR partition records

[James Clarke]

Since the MBR layout only has partition records as 2-byte aligned, the 32-bit
fields in them are not aligned. Thus, they cannot be accessed on some
architectures (such as SPARC) by using a "struct MBR_part_record *" pointer,
as the compiler can assume that the pointer is properly aligned. Instead, the
records must be accessed by going through the MBR struct itself every time.

Signed-off-by: James Clarke <jrtc27@jrtc27.com>
---
Changes from v1:

 * Added packed attribute to MBR_part_record

 * Reformatted changes to fit the 80-character line limit (assuming a tab stop
   of 4; if it's 8 I can try and cut the line lengths down, though with the
   extra verbosity that's going to be more awkward...)

 part.h      |  2 +-
 super-mbr.c |  6 ++++++
 util.c      | 15 ++++++++-------
 3 files changed, 15 insertions(+), 8 deletions(-)

diff --git a/part.h b/part.h
index 862a14c..e697fb4 100644
--- a/part.h
+++ b/part.h
@@ -40,7 +40,7 @@ struct MBR_part_record {
   __u8 last_cyl;
   __u32 first_sect_lba;
   __u32 blocks_num;
-};
+} __attribute__((packed));
 
 struct MBR {
 	__u8 pad[446];
diff --git a/super-mbr.c b/super-mbr.c
index 62b3f03..303dde4 100644
--- a/super-mbr.c
+++ b/super-mbr.c
@@ -57,6 +57,9 @@ static void examine_mbr(struct supertype *st, char *homehost)
 
 	printf("   MBR Magic : %04x\n", sb->magic);
 	for (i = 0; i < MBR_PARTITIONS; i++)
+		/* Have to make every access through sb rather than using a pointer to
+		 * the partition table (or an entry), since the entries are not
+		 * properly aligned. */
 		if (sb->parts[i].blocks_num)
 			printf("Partition[%d] : %12lu sectors at %12lu (type %02x)\n",
 			       i,
@@ -151,6 +154,9 @@ static void getinfo_mbr(struct supertype *st, struct mdinfo *info, char *map)
 	info->component_size = 0;
 
 	for (i = 0; i < MBR_PARTITIONS ; i++)
+		/* Have to make every access through sb rather than using a pointer to
+		 * the partition table (or an entry), since the entries are not
+		 * properly aligned. */
 		if (sb->parts[i].blocks_num) {
 			unsigned long last =
 				(unsigned long)__le32_to_cpu(sb->parts[i].blocks_num)
diff --git a/util.c b/util.c
index a238a21..4a8767b 100644
--- a/util.c
+++ b/util.c
@@ -1412,7 +1412,6 @@ static int get_gpt_last_partition_end(int fd, unsigned long long *endofpart)
 static int get_last_partition_end(int fd, unsigned long long *endofpart)
 {
 	struct MBR boot_sect;
-	struct MBR_part_record *part;
 	unsigned long long curr_part_end;
 	unsigned part_nr;
 	int retval = 0;
@@ -1429,21 +1428,23 @@ static int get_last_partition_end(int fd, unsigned long long *endofpart)
 	if (boot_sect.magic == MBR_SIGNATURE_MAGIC) {
 		retval = 1;
 		/* found the correct signature */
-		part = boot_sect.parts;
 
 		for (part_nr = 0; part_nr < MBR_PARTITIONS; part_nr++) {
+			/* Have to make every access through boot_sect rather than using a
+			 * pointer to the partition table (or an entry), since the entries
+			 * are not properly aligned. */
+
 			/* check for GPT type */
-			if (part->part_type == MBR_GPT_PARTITION_TYPE) {
+			if (boot_sect.parts[part_nr].part_type == MBR_GPT_PARTITION_TYPE) {
 				retval = get_gpt_last_partition_end(fd, endofpart);
 				break;
 			}
 			/* check the last used lba for the current partition  */
-			curr_part_end = __le32_to_cpu(part->first_sect_lba) +
-				__le32_to_cpu(part->blocks_num);
+			curr_part_end =
+				__le32_to_cpu(boot_sect.parts[part_nr].first_sect_lba) +
+				__le32_to_cpu(boot_sect.parts[part_nr].blocks_num);
 			if (curr_part_end > *endofpart)
 				*endofpart = curr_part_end;
-
-			part++;
 		}
 	} else {
 		/* Unknown partition table */
-- 
2.10.1

Re: [PATCH v2] Fix bus error when accessing MBR partition records

[Jes Sorensen]

James Clarke <jrtc27@jrtc27.com> writes:
> Since the MBR layout only has partition records as 2-byte aligned, the 32-bit
> fields in them are not aligned. Thus, they cannot be accessed on some
> architectures (such as SPARC) by using a "struct MBR_part_record *" pointer,
> as the compiler can assume that the pointer is properly aligned. Instead, the
> records must be accessed by going through the MBR struct itself every time.
>
> Signed-off-by: James Clarke <jrtc27@jrtc27.com>
> ---
> Changes from v1:
>
>  * Added packed attribute to MBR_part_record
>
>  * Reformatted changes to fit the 80-character line limit (assuming a tab stop
>    of 4; if it's 8 I can try and cut the line lengths down, though with the
>    extra verbosity that's going to be more awkward...)

I am going to clean this up - a tab is always 8 characters just like
with the kernel.

Jes

>
>  part.h      |  2 +-
>  super-mbr.c |  6 ++++++
>  util.c      | 15 ++++++++-------
>  3 files changed, 15 insertions(+), 8 deletions(-)
>
> diff --git a/part.h b/part.h
> index 862a14c..e697fb4 100644
> --- a/part.h
> +++ b/part.h
> @@ -40,7 +40,7 @@ struct MBR_part_record {
>    __u8 last_cyl;
>    __u32 first_sect_lba;
>    __u32 blocks_num;
> -};
> +} __attribute__((packed));
>  
>  struct MBR {
>  	__u8 pad[446];
> diff --git a/super-mbr.c b/super-mbr.c
> index 62b3f03..303dde4 100644
> --- a/super-mbr.c
> +++ b/super-mbr.c
> @@ -57,6 +57,9 @@ static void examine_mbr(struct supertype *st, char *homehost)
>  
>  	printf("   MBR Magic : %04x\n", sb->magic);
>  	for (i = 0; i < MBR_PARTITIONS; i++)
> +		/* Have to make every access through sb rather than using a pointer to
> +		 * the partition table (or an entry), since the entries are not
> +		 * properly aligned. */
>  		if (sb->parts[i].blocks_num)
>  			printf("Partition[%d] : %12lu sectors at %12lu (type %02x)\n",
>  			       i,
> @@ -151,6 +154,9 @@ static void getinfo_mbr(struct supertype *st, struct mdinfo *info, char *map)
>  	info->component_size = 0;
>  
>  	for (i = 0; i < MBR_PARTITIONS ; i++)
> +		/* Have to make every access through sb rather than using a pointer to
> +		 * the partition table (or an entry), since the entries are not
> +		 * properly aligned. */
>  		if (sb->parts[i].blocks_num) {
>  			unsigned long last =
>  				(unsigned long)__le32_to_cpu(sb->parts[i].blocks_num)
> diff --git a/util.c b/util.c
> index a238a21..4a8767b 100644
> --- a/util.c
> +++ b/util.c
> @@ -1412,7 +1412,6 @@ static int get_gpt_last_partition_end(int fd, unsigned long long *endofpart)
>  static int get_last_partition_end(int fd, unsigned long long *endofpart)
>  {
>  	struct MBR boot_sect;
> -	struct MBR_part_record *part;
>  	unsigned long long curr_part_end;
>  	unsigned part_nr;
>  	int retval = 0;
> @@ -1429,21 +1428,23 @@ static int get_last_partition_end(int fd, unsigned long long *endofpart)
>  	if (boot_sect.magic == MBR_SIGNATURE_MAGIC) {
>  		retval = 1;
>  		/* found the correct signature */
> -		part = boot_sect.parts;
>  
>  		for (part_nr = 0; part_nr < MBR_PARTITIONS; part_nr++) {
> +			/* Have to make every access through boot_sect rather than using a
> +			 * pointer to the partition table (or an entry), since the entries
> +			 * are not properly aligned. */
> +
>  			/* check for GPT type */
> -			if (part->part_type == MBR_GPT_PARTITION_TYPE) {
> +			if (boot_sect.parts[part_nr].part_type == MBR_GPT_PARTITION_TYPE) {
>  				retval = get_gpt_last_partition_end(fd, endofpart);
>  				break;
>  			}
>  			/* check the last used lba for the current partition  */
> -			curr_part_end = __le32_to_cpu(part->first_sect_lba) +
> -				__le32_to_cpu(part->blocks_num);
> +			curr_part_end =
> +				__le32_to_cpu(boot_sect.parts[part_nr].first_sect_lba) +
> +				__le32_to_cpu(boot_sect.parts[part_nr].blocks_num);
>  			if (curr_part_end > *endofpart)
>  				*endofpart = curr_part_end;
> -
> -			part++;
>  		}
>  	} else {
>  		/* Unknown partition table */