[PATCH v3] wic: ensure generated disk system identifier is non-zero

[PATCH v3] wic: ensure generated disk system identifier is non-zero

[Jonathan Liu]

Zero may be interpreted as no MBR signature present and another
partitioning program might install a new MBR signature.

Signed-off-by: Jonathan Liu <net147@gmail.com>
---
 scripts/lib/wic/plugins/imager/direct.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/scripts/lib/wic/plugins/imager/direct.py b/scripts/lib/wic/plugins/imager/direct.py
index f20d8433f1..fe9b688ab0 100644
--- a/scripts/lib/wic/plugins/imager/direct.py
+++ b/scripts/lib/wic/plugins/imager/direct.py
@@ -297,7 +297,7 @@ class PartitionedImage():
                           # all partitions (in bytes)
         self.ptable_format = ptable_format  # Partition table format
         # Disk system identifier
-        self.identifier = int.from_bytes(os.urandom(4), 'little')
+        self.identifier = int.from_bytes(os.urandom(4), 'little') or 0xffffffff
 
         self.partitions = partitions
         self.partimages = []
-- 
2.13.2

Re: [PATCH v3] wic: ensure generated disk system identifier is non-zero

[Ed Bartosh]

On Sat, Jul 29, 2017 at 12:45:27AM +1000, Jonathan Liu wrote:
> Zero may be interpreted as no MBR signature present and another
> partitioning program might install a new MBR signature.
> 
> Signed-off-by: Jonathan Liu <net147@gmail.com>
> ---
>  scripts/lib/wic/plugins/imager/direct.py | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/scripts/lib/wic/plugins/imager/direct.py b/scripts/lib/wic/plugins/imager/direct.py
> index f20d8433f1..fe9b688ab0 100644
> --- a/scripts/lib/wic/plugins/imager/direct.py
> +++ b/scripts/lib/wic/plugins/imager/direct.py
> @@ -297,7 +297,7 @@ class PartitionedImage():
>                            # all partitions (in bytes)
>          self.ptable_format = ptable_format  # Partition table format
>          # Disk system identifier
> -        self.identifier = int.from_bytes(os.urandom(4), 'little')
> +        self.identifier = int.from_bytes(os.urandom(4), 'little') or 0xffffffff
>  

Can we generate random identifier by calling os.urandom again if
self.identifier is zero? Something like this, may be?

while True:
    self.identifier = int.from_bytes(os.urandom(4), 'little')
    if self.identifier:
        break

Assigning constant 0xffffffff can potentially create nonunique identifiers.

>          self.partitions = partitions
>          self.partimages = []
> -- 
> 2.13.2
> 
> -- 
> _______________________________________________
> Openembedded-core mailing list
> Openembedded-core@lists.openembedded.org
> http://lists.openembedded.org/mailman/listinfo/openembedded-core

-- 
--
Regards,
Ed

Re: [PATCH v3] wic: ensure generated disk system identifier is non-zero

[Jonathan Liu]

Hi Ed,

On 30 July 2017 at 20:02, Ed Bartosh <ed.bartosh@linux.intel.com> wrote:
> On Sat, Jul 29, 2017 at 12:45:27AM +1000, Jonathan Liu wrote:
>> Zero may be interpreted as no MBR signature present and another
>> partitioning program might install a new MBR signature.
>>
>> Signed-off-by: Jonathan Liu <net147@gmail.com>
>> ---
>>  scripts/lib/wic/plugins/imager/direct.py | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/scripts/lib/wic/plugins/imager/direct.py b/scripts/lib/wic/plugins/imager/direct.py
>> index f20d8433f1..fe9b688ab0 100644
>> --- a/scripts/lib/wic/plugins/imager/direct.py
>> +++ b/scripts/lib/wic/plugins/imager/direct.py
>> @@ -297,7 +297,7 @@ class PartitionedImage():
>>                            # all partitions (in bytes)
>>          self.ptable_format = ptable_format  # Partition table format
>>          # Disk system identifier
>> -        self.identifier = int.from_bytes(os.urandom(4), 'little')
>> +        self.identifier = int.from_bytes(os.urandom(4), 'little') or 0xffffffff
>>
>
> Can we generate random identifier by calling os.urandom again if
> self.identifier is zero? Something like this, may be?
>
> while True:
>     self.identifier = int.from_bytes(os.urandom(4), 'little')
>     if self.identifier:
>         break
>
> Assigning constant 0xffffffff can potentially create nonunique identifiers.
>

There is no guarantee that urandom will create unique identifiers.
Infinite loop needs a timeout and error in the case it is unable to
generate a suitable identifier. It is only 0xffffffff if it is 0. That
means 0xffffffff is twice as likely to occur (2 in 4294967296 instead
of 1 in 4294967296) so there is tiny bias. Note that this is how it is
implemented for the DISK_SIGNATURE variable in OE and in GNU Parted as
well.

>>          self.partitions = partitions
>>          self.partimages = []
>> --
>> 2.13.2
>>
>> --
>> _______________________________________________
>> Openembedded-core mailing list
>> Openembedded-core@lists.openembedded.org
>> http://lists.openembedded.org/mailman/listinfo/openembedded-core
>
> --
> --
> Regards,
> Ed

Regards,
Jonathan

Re: [PATCH v3] wic: ensure generated disk system identifier is non-zero

[Ed Bartosh]

On Sun, Jul 30, 2017 at 08:37:26PM +1000, Jonathan Liu wrote:
> Hi Ed,
> 
> On 30 July 2017 at 20:02, Ed Bartosh <ed.bartosh@linux.intel.com> wrote:
> > On Sat, Jul 29, 2017 at 12:45:27AM +1000, Jonathan Liu wrote:
> >> Zero may be interpreted as no MBR signature present and another
> >> partitioning program might install a new MBR signature.
> >>
> >> Signed-off-by: Jonathan Liu <net147@gmail.com>
> >> ---
> >>  scripts/lib/wic/plugins/imager/direct.py | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/scripts/lib/wic/plugins/imager/direct.py b/scripts/lib/wic/plugins/imager/direct.py
> >> index f20d8433f1..fe9b688ab0 100644
> >> --- a/scripts/lib/wic/plugins/imager/direct.py
> >> +++ b/scripts/lib/wic/plugins/imager/direct.py
> >> @@ -297,7 +297,7 @@ class PartitionedImage():
> >>                            # all partitions (in bytes)
> >>          self.ptable_format = ptable_format  # Partition table format
> >>          # Disk system identifier
> >> -        self.identifier = int.from_bytes(os.urandom(4), 'little')
> >> +        self.identifier = int.from_bytes(os.urandom(4), 'little') or 0xffffffff
> >>
> >
> > Can we generate random identifier by calling os.urandom again if
> > self.identifier is zero? Something like this, may be?
> >
> > while True:
> >     self.identifier = int.from_bytes(os.urandom(4), 'little')
> >     if self.identifier:
> >         break
> >
> > Assigning constant 0xffffffff can potentially create nonunique identifiers.
> >
>
> There is no guarantee that urandom will create unique identifiers.
We can't have a guarantee with urandom, true. My point was that if you
need random value it's better to call urandom again than using a constant value.

> Infinite loop needs a timeout and error in the case it is unable to
> generate a suitable identifier.
I'm not sure it's needed, but it's not a big deal to add this. I really
doubt os.urandom can generate long enough sequence of zeros.

> It is only 0xffffffff if it is 0. That
> means 0xffffffff is twice as likely to occur (2 in 4294967296 instead
> of 1 in 4294967296) so there is tiny bias. Note that this is how it is
> implemented for the DISK_SIGNATURE variable in OE and in GNU Parted as
> well.
Does this mean we shouldn't do better?

> >>          self.partitions = partitions
> >>          self.partimages = []
> >> --
> >> 2.13.2
> >>
> >> --
> >> _______________________________________________
> >> Openembedded-core mailing list
> >> Openembedded-core@lists.openembedded.org
> >> http://lists.openembedded.org/mailman/listinfo/openembedded-core
>
> Regards,
> Jonathan

--
Regards,
Ed

Re: [PATCH v3] wic: ensure generated disk system identifier is non-zero

[Jonathan Liu]

Hi Ed,

On 31 July 2017 at 17:28, Ed Bartosh <ed.bartosh@linux.intel.com> wrote:
> On Sun, Jul 30, 2017 at 08:37:26PM +1000, Jonathan Liu wrote:
>> Hi Ed,
>>
>> On 30 July 2017 at 20:02, Ed Bartosh <ed.bartosh@linux.intel.com> wrote:
>> > On Sat, Jul 29, 2017 at 12:45:27AM +1000, Jonathan Liu wrote:
>> >> Zero may be interpreted as no MBR signature present and another
>> >> partitioning program might install a new MBR signature.
>> >>
>> >> Signed-off-by: Jonathan Liu <net147@gmail.com>
>> >> ---
>> >>  scripts/lib/wic/plugins/imager/direct.py | 2 +-
>> >>  1 file changed, 1 insertion(+), 1 deletion(-)
>> >>
>> >> diff --git a/scripts/lib/wic/plugins/imager/direct.py b/scripts/lib/wic/plugins/imager/direct.py
>> >> index f20d8433f1..fe9b688ab0 100644
>> >> --- a/scripts/lib/wic/plugins/imager/direct.py
>> >> +++ b/scripts/lib/wic/plugins/imager/direct.py
>> >> @@ -297,7 +297,7 @@ class PartitionedImage():
>> >>                            # all partitions (in bytes)
>> >>          self.ptable_format = ptable_format  # Partition table format
>> >>          # Disk system identifier
>> >> -        self.identifier = int.from_bytes(os.urandom(4), 'little')
>> >> +        self.identifier = int.from_bytes(os.urandom(4), 'little') or 0xffffffff
>> >>
>> >
>> > Can we generate random identifier by calling os.urandom again if
>> > self.identifier is zero? Something like this, may be?
>> >
>> > while True:
>> >     self.identifier = int.from_bytes(os.urandom(4), 'little')
>> >     if self.identifier:
>> >         break
>> >
>> > Assigning constant 0xffffffff can potentially create nonunique identifiers.
>> >
>>
>> There is no guarantee that urandom will create unique identifiers.
> We can't have a guarantee with urandom, true. My point was that if you
> need random value it's better to call urandom again than using a constant value.
>
>> Infinite loop needs a timeout and error in the case it is unable to
>> generate a suitable identifier.
> I'm not sure it's needed, but it's not a big deal to add this. I really
> doubt os.urandom can generate long enough sequence of zeros.
>
>> It is only 0xffffffff if it is 0. That
>> means 0xffffffff is twice as likely to occur (2 in 4294967296 instead
>> of 1 in 4294967296) so there is tiny bias. Note that this is how it is
>> implemented for the DISK_SIGNATURE variable in OE and in GNU Parted as
>> well.
> Does this mean we shouldn't do better?
>

How about random.SystemRandom().randrange(1, 0xffffffff) ?

>> >>          self.partitions = partitions
>> >>          self.partimages = []
>> >> --
>> >> 2.13.2
>> >>
>> >> --
>> >> _______________________________________________
>> >> Openembedded-core mailing list
>> >> Openembedded-core@lists.openembedded.org
>> >> http://lists.openembedded.org/mailman/listinfo/openembedded-core
>>
>> Regards,
>> Jonathan
>
> --
> Regards,
> Ed

Regards,
Jonathan

Re: [PATCH v3] wic: ensure generated disk system identifier is non-zero

[Jonathan Liu]

On 31 July 2017 at 17:58, Jonathan Liu <net147@gmail.com> wrote:
> Hi Ed,
>
> On 31 July 2017 at 17:28, Ed Bartosh <ed.bartosh@linux.intel.com> wrote:
>> On Sun, Jul 30, 2017 at 08:37:26PM +1000, Jonathan Liu wrote:
>>> Hi Ed,
>>>
>>> On 30 July 2017 at 20:02, Ed Bartosh <ed.bartosh@linux.intel.com> wrote:
>>> > On Sat, Jul 29, 2017 at 12:45:27AM +1000, Jonathan Liu wrote:
>>> >> Zero may be interpreted as no MBR signature present and another
>>> >> partitioning program might install a new MBR signature.
>>> >>
>>> >> Signed-off-by: Jonathan Liu <net147@gmail.com>
>>> >> ---
>>> >>  scripts/lib/wic/plugins/imager/direct.py | 2 +-
>>> >>  1 file changed, 1 insertion(+), 1 deletion(-)
>>> >>
>>> >> diff --git a/scripts/lib/wic/plugins/imager/direct.py b/scripts/lib/wic/plugins/imager/direct.py
>>> >> index f20d8433f1..fe9b688ab0 100644
>>> >> --- a/scripts/lib/wic/plugins/imager/direct.py
>>> >> +++ b/scripts/lib/wic/plugins/imager/direct.py
>>> >> @@ -297,7 +297,7 @@ class PartitionedImage():
>>> >>                            # all partitions (in bytes)
>>> >>          self.ptable_format = ptable_format  # Partition table format
>>> >>          # Disk system identifier
>>> >> -        self.identifier = int.from_bytes(os.urandom(4), 'little')
>>> >> +        self.identifier = int.from_bytes(os.urandom(4), 'little') or 0xffffffff
>>> >>
>>> >
>>> > Can we generate random identifier by calling os.urandom again if
>>> > self.identifier is zero? Something like this, may be?
>>> >
>>> > while True:
>>> >     self.identifier = int.from_bytes(os.urandom(4), 'little')
>>> >     if self.identifier:
>>> >         break
>>> >
>>> > Assigning constant 0xffffffff can potentially create nonunique identifiers.
>>> >
>>>
>>> There is no guarantee that urandom will create unique identifiers.
>> We can't have a guarantee with urandom, true. My point was that if you
>> need random value it's better to call urandom again than using a constant value.
>>
>>> Infinite loop needs a timeout and error in the case it is unable to
>>> generate a suitable identifier.
>> I'm not sure it's needed, but it's not a big deal to add this. I really
>> doubt os.urandom can generate long enough sequence of zeros.
>>
>>> It is only 0xffffffff if it is 0. That
>>> means 0xffffffff is twice as likely to occur (2 in 4294967296 instead
>>> of 1 in 4294967296) so there is tiny bias. Note that this is how it is
>>> implemented for the DISK_SIGNATURE variable in OE and in GNU Parted as
>>> well.
>> Does this mean we shouldn't do better?
>>
>
> How about random.SystemRandom().randrange(1, 0xffffffff) ?
>

random.SystemRandom().randint(1, 0xffffffff) actually

>>> >>          self.partitions = partitions
>>> >>          self.partimages = []
>>> >> --
>>> >> 2.13.2
>>> >>
>>> >> --
>>> >> _______________________________________________
>>> >> Openembedded-core mailing list
>>> >> Openembedded-core@lists.openembedded.org
>>> >> http://lists.openembedded.org/mailman/listinfo/openembedded-core
>>>
>>> Regards,
>>> Jonathan
>>
>> --
>> Regards,
>> Ed
>
> Regards,
> Jonathan

Re: [PATCH v3] wic: ensure generated disk system identifier is non-zero

[Ed Bartosh]

On Mon, Jul 31, 2017 at 06:04:47PM +1000, Jonathan Liu wrote:
> >
> > How about random.SystemRandom().randrange(1, 0xffffffff) ?
> >
> 
> random.SystemRandom().randint(1, 0xffffffff) actually
> 

This looks ok to me. Thanks.

--
Regards,
Ed