[U-Boot] [PATCH V2 2/3] lib: uuid: add functions to generate UUID version 4

[Tom Rini]

On Thu, Mar 13, 2014 at 07:41:24PM +0100, Wolfgang Denk wrote:
> Dear Przemyslaw Marczak,
> 
> In message <cc0f558724a4d3ea3497b84601038f5f18f37a7b.1394037321.git.p.marczak at samsung.com> you wrote:
> > This patch adds support to generate UUID (Universally Unique Identifier)
> > in version 4 based on RFC4122, which is randomly.
> ...
> > +struct uuid {
> > +	unsigned int time_low;
> > +	unsigned short time_mid;
> > +	unsigned short time_hi_and_version;
> > +	unsigned char clock_seq_hi_and_reserved;
> > +	unsigned char clock_seq_low;
> > +	unsigned char node[6];
> > +};
> 
> This struct starts with an uint, so it requires alignment on a 32 bit
> boundary (i. e. an address that is a multiple of 4).

And this needs to be marked as packed since we're using this as a direct
representation of things on-disk.

> > +void gen_rand_uuid(unsigned char *uuid_bin)
> > +{
> > +	struct uuid *uuid = (struct uuid *)uuid_bin;
> 
> Here you cast a pointer to the (unaligned) character buffer to a
> struct buffer, which requires alignment.

Potentially unaligned buffer.  There's only one caller thus far, and it
will be aligned there.  We do need to comment that the pointer needs to
be aligned.

> > +	unsigned int *ptr = (unsigned int *)uuid_bin;
> 
> > +	/* Set all fields randomly */
> > +	for (i = 0; i < sizeof(struct uuid) / sizeof(*ptr); i++)
> > +		*(ptr + i) = rand();
> 
> This code is dangerous - if the size of the struct should not be a
> multiple of sizeof(uint), there would remain uninitialized data.

With the struct not packed, it'll be padded out so this works.  But
looking at how we later use this as I say above, we do need to pack it,
and then this will not be safe.  Some looping of strncpy into the char
buffer, as a char so we whack the rand data in?

> > +	/* Set V4 format */
> > +	uuid->time_hi_and_version &= UUID_VERSION_CLEAR_BITS;
> > +	uuid->time_hi_and_version |= UUID_VERSION << UUID_VERSION_SHIFT;
> 
> Potentially unaligned accesses.

As-is no (hidden padding), with packed yes-but-handled (compiler can see
this too, will behave correctly).

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20140313/3b92cfa8/attachment.pgp>