[PATCH V2] dos: copy previous bootsector for dos partition table

[PATCH V2] dos: copy previous bootsector for dos partition table

[Jean-Loup 'clippix' Bogalho]

In order to create a dos partition scheme, libfdisk erase the first disk sector
and then add the needed values (partitions, magic numbers).
In the case the disk already contains code in the bootsection, it will be
erased.  This was not the case in v2.25 of util-linux.
Since fdisk can be used to repair messed partition tables, maybe we don't want
to have to rewrite the boot section.
If you want to keep the precedent behavior, here's a patch that re-copy the old
bootsection after it has been wiped. There is other ways to do it, but this one
doesn't interfere with the rest of the code.

Signed-off-by: Jean-Loup 'clippix' Bogalho <clippix@lse.epita.fr>
---
 libfdisk/src/dos.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/libfdisk/src/dos.c b/libfdisk/src/dos.c
index dce9829..998080a 100644
--- a/libfdisk/src/dos.c
+++ b/libfdisk/src/dos.c
@@ -18,6 +18,7 @@
 
 #define MAXIMUM_PARTS	60
 #define ACTIVE_FLAG     0x80
+#define BOOTSECTOR_SIZE	446
 
 /**
  * SECTION: dos
@@ -659,9 +660,17 @@ static int dos_create_disklabel(struct fdisk_context *cxt)
 	if (!has_id)
 		random_get_bytes(&id, sizeof(id));
 
-	rc = fdisk_init_firstsector_buffer(cxt);
-	if (rc)
-		return rc;
+	if (cxt->firstsector && cxt->firstsector_bufsz == cxt->sector_size) {
+		unsigned char bootsector[BOOTSECTOR_SIZE];
+		memcpy(bootsector, cxt->firstsector, BOOTSECTOR_SIZE);
+		rc = fdisk_init_firstsector_buffer(cxt);
+		memcpy(cxt->firstsector, bootsector, BOOTSECTOR_SIZE);
+	}
+	else {
+		rc = fdisk_init_firstsector_buffer(cxt);
+		if (rc)
+			return rc;
+	}
 	dos_init(cxt);
 
 	l = self_label(cxt);

-- 
Jean-Loup 'clippix' Bogalho
LSE 2016

Re: [PATCH V2] dos: copy previous bootsector for dos partition table

[Karel Zak]

On Sat, Apr 04, 2015 at 06:05:26PM +0200, Jean-Loup 'clippix' Bogalho wrote:
> In order to create a dos partition scheme, libfdisk erase the first disk sector
> and then add the needed values (partitions, magic numbers).
> In the case the disk already contains code in the bootsection, it will be
> erased.  

 Yes.

> This was not the case in v2.25 of util-linux.

 Are you sure? I see fdisk_init_firstsector_buffer() in 

    git show v2.25:libfdisk/src/utils.c

 and we zeroize the first sector all time, for example see
 create_doslabel() and get_boot() from 

    git show v2.13:fdisk/fdisk.c


 I have tested v2.17:

 # hexdump -C /dev/sdc
 00000000  48 45 4c 4c 4f 0a 00 00  00 00 00 00 00 00 00 00  |HELLO...........|
 00000010  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  |................|
 *

 # fdisk /dev/sdc
 ... create MBR ....


 # hexdump -C /dev/sdc
 00000000  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  |................|
 *
 000001b0  00 00 00 00 00 00 00 00  b4 19 30 de 00 00 00 01  |..........0.....|
 000001c0  01 00 83 10 fb fb 3b 00  00 00 19 9c 0f 00 00 00  |......;.........|
 000001d0  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  |................|
 *
 000001f0  00 00 00 00 00 00 00 00  00 00 00 00 00 00 55 aa  |..............U.|


> Since fdisk can be used to repair messed partition tables, maybe we don't want
> to have to rewrite the boot section.

 That's question. If you keep the begin of the device unchanged then
 you can create horrible mix of more labels or you can keep any unwanted
 filesystem or raid superblock on the device. I don't think we want this 
 by default. (Well, in many cases this problem should be detected by 
 warn_wipe() in context.c.)

 Note that GNU parted also modifies the begin of the device (it
 tries to be smart and it writes some generic "boot code" there:-)
 
 Maybe it would be better to add --protect-bootsector command line option;
 if you want to implement it:

    - add protect_boot flag to struct fdisk_context (fdiskP.h)
    - add a new function fdisk_protect_boot(cxt, enable) to set/unset the new flag

    - extend fdisk_init_firstsector_buffer() to protect specified area:
         fdisk_init_firstsector_buffer(cxt, 0, 446)

    - call fdisk_init_firstsector_buffer() from dos_create_disklabel()
      with proper offset and size when the protect_boot flag is enabled

    - add fdisk_protect_boot() to 
        libfdisk/src/libfdisk.h.in
        libfdisk/src/libfdisk.sym
        libfdisk/docs/libfdisk-sections.txt

  Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: [PATCH V2] dos: copy previous bootsector for dos partition table

[Karel Zak]

On Tue, Apr 07, 2015 at 11:45:35AM +0200, Karel Zak wrote:
>  Maybe it would be better to add --protect-bootsector command line option;
>  if you want to implement it:
> 
>     - add protect_boot flag to struct fdisk_context (fdiskP.h)
>     - add a new function fdisk_protect_boot(cxt, enable) to set/unset the new flag
> 
>     - extend fdisk_init_firstsector_buffer() to protect specified area:
>          fdisk_init_firstsector_buffer(cxt, 0, 446)
> 
>     - call fdisk_init_firstsector_buffer() from dos_create_disklabel()
>       with proper offset and size when the protect_boot flag is enabled
> 
>     - add fdisk_protect_boot() to 
>         libfdisk/src/libfdisk.h.in
>         libfdisk/src/libfdisk.sym
>         libfdisk/docs/libfdisk-sections.txt

I'm working on this, because it's necessary for sfdisk.

    Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com