Generate CRC of structure of Global Data for standalone apps

There has been some discussion of late on the mailing list as to whether or not a change to the Global Data structure should trigger an increment to XF_VERSION. The issue is complicated by the fact that some arches have #ifdef's in the definition of the structure.

This patch auto-generates a CRC32 checksum of the structure (as opposed to the contents) of the Global Data structure. Standalone applications can read U-Boot's CRC via the new exported get_gd_crc(). If the CRCs do not match, using Global Data in the standalone application would be very unwise.

This method allows the global data structure to be modified without having to worry about whether existing standalone applications will suddenly fail with unexpected results when accessing global data - Of course, this will only work for standalone applications which actually use get_gd_crc() - By incrementing XF_VERSION, all existing standalone application (should) fail gracefully on a version mismatch anyway

The CRC is generated by:
- Pre-processing common.h (which includes global_data.h after all the board, arch and SoC defines are set)
- Searches for a chunk of text delimited by 'typedef struct global_data {' and '} gd_t;'
- Strips all blank lines and whitespaces (pre-processor already took care of comments)
- Pipes the result through 'tools/gencrc32header/gencrc32header'

NOTE: There is something a bit funny about the make dependencies for gencrc32header - Changes to gencrc32header.c do not trigger a rebuild

---
 Makefile                              |   14 +++++-
 common/exports.c                      |    6 +++
 examples/standalone/hello_world.c     |    6 +++
 include/_exports.h                    |    1 +
 include/exports.h                     |    3 +-
 tools/gencrc32header/Makefile         |   62 +++++++++++++++++++++++++
 tools/gencrc32header/gencrc32header.c |   80 +++++++++++++++++++++++++++++++++
 7 files changed, 170 insertions(+), 2 deletions(-)
 create mode 100644 tools/gencrc32header/Makefile
 create mode 100644 tools/gencrc32header/gencrc32header.c

index 049b169..df61b02 100644
@@ -465,7 +465,8 @@ updater:
 depend dep:	$(TIMESTAMP_FILE) $(VERSION_FILE) \
 		$(obj)include/autoconf.mk \
 		$(obj)include/generated/generic-asm-offsets.h \
-		$(obj)include/asm/arch/asm-offsets.h
+		$(obj)include/asm/arch/asm-offsets.h \
+		$(obj)include/global_data_crc.h
 		for dir in $(SUBDIRS) $(CPUDIR) $(dir $(LDSCRIPT)) ; do \
 			$(MAKE) -C $$dir _depend ; done
 

@@ -545,6 +546,17 @@ $(TOPDIR)$(CPUDIR)/$(SOC)/asm-offsets.s:	$(TOPDIR)/include/autoconf.mk.dep \
 		-o $@ $(TOPDIR)/$(CPUDIR)/$(SOC)/asm-offsets.c -c -S \
 	, at echo $(TOPDIR)/$(CPUDIR)/$(SOC)/asm-offsets.c does not exist - skipping)
 
+$(obj)include/global_data_crc.h:	$(obj)tools/gencrc32header/gencrc32header \
+	$(src)include/asm/global_data.h
+	$(CPP) $(CPPFLAGS) -E $(TOPDIR)/include/common.h | \
+	sed -ne '/^typedef\s*struct\s*global_data\s*{/,/^}\s*gd_t;/p' | \
+	sed '/^$$/d' | \
+	sed 's/[ ]//g' | \
+	$(obj)tools/gencrc32header/gencrc32header > $@
+
+$(obj)tools/gencrc32header/gencrc32header:
+	$(MAKE) -C tools/gencrc32header/ all
+
 #########################################################################
 else	# !config.mk
 all $(obj)u-boot.hex $(obj)u-boot.srec $(obj)u-boot.bin \

index 717e4af..8e767cc 100644
@@ -1,5 +1,6 @@
 #include <common.h>
 #include <exports.h>
+#include <global_data_crc.h>
 
 DECLARE_GLOBAL_DATA_PTR;
 

@@ -12,6 +13,11 @@ unsigned long get_version(void)
 	return XF_VERSION;
 }
 
+unsigned long get_gd_crc(void)
+{
+	return XF_GLOBAL_DATA_CRC;
+}
+
 /* Reuse _exports.h with a little trickery to avoid bitrot */
 #define EXPORT_FUNC(sym) gd->jt[XF_##sym] = (void *)sym;
 

index 067c390..18e3a51 100644
@@ -23,6 +23,7 @@
 
 #include <common.h>
 #include <exports.h>
+#include <global_data_crc.h>
 
 int hello_world (int argc, char * const argv[])
 {

@@ -33,6 +34,11 @@ int hello_world (int argc, char * const argv[])
 	printf ("Example expects ABI version %d\n", XF_VERSION);
 	printf ("Actual U-Boot ABI version %d\n", (int)get_version());
 
+	printf ("Example expects Global Data Structure CRC 0x%8.8lx\n",
+			XF_GLOBAL_DATA_CRC);
+	printf ("Actual U-Boot Global Data Structure CRC 0x%8.8lx\n\n",
+			get_gd_crc());
+
 	printf ("Hello World\n");
 
 	printf ("argc = %d\n", argc);

index 349a3c5..6ffa7b5 100644
@@ -3,6 +3,7 @@
  * in the final configuration (such as i2c).
  */
 EXPORT_FUNC(get_version)
+EXPORT_FUNC(get_gd_crc)
 EXPORT_FUNC(getc)
 EXPORT_FUNC(tstc)
 EXPORT_FUNC(putc)

index 9492566..fc11a18 100644
@@ -7,6 +7,7 @@
 
 /* These are declarations of exported functions available in C code */
 unsigned long get_version(void);
+unsigned long get_gd_crc(void);
 int  getc(void);
 int  tstc(void);
 void putc(const char);

@@ -44,7 +45,7 @@ enum {
 	XF_MAX
 };
 
-#define XF_VERSION	6
+#define XF_VERSION	7
 
 #if defined(CONFIG_X86)
 extern gd_t *global_data;

--
1.7.5.2.317.g391b14

Dear Graeme Russ,

Sorry, but I don't consider this an even halfway robust or reliable method. Minor changes to the text formatting, changes to remove for example the typedef, renames of fields or appending additional fields will all break this, while tere will actually be no problems with the code.

THis is a level of make-believe security that is fragile, and I doubt if it's really useful, or needed.

It's a nice idea, but the problem it addresses is mostly a theoretical one - has anybody seen any actual bug reports that this has ever been a real problem?

Best regards,

Wolfgang Denk

DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
... The prejudices people feel about each other disappear  when  then
get to know each other.
	-- Kirk, "Elaan of Troyius", stardate 4372.5

Hi Wolfgang,

Back the original question that spawned this - asked several times and never answered - Does a modification to the Global Data structure trigger an increment of XF_VERSION?

If the answer is No, then no problems, dropping this is OK by me

However, if the answer is 'yes', what do we do about the #ifdefs?

Regards,

Graeme

Dear Graeme Russ,

I don't think so. But I have to admit that I never thoroughly verifyed this either. The standalone code itself is not suposed to reference GD in any way; and the U-Boot routines that the standalone call can call are runnign in the context of the current U-Boot, where everything fits together. So unless somebody misuses something, it should all work out well. At least, that's my theory.

Best regards,

Wolfgang Denk

DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
I think there's a world market for about five computers.
         -- attr. Thomas J. Watson (Chairman of the Board, IBM), 1943

So perhaps this should be removed from exports.h:

#if defined(CONFIG_X86)
extern gd_t *global_data;
#endif

And document that standalone applications must not directly access the global data structure.

The whole point of the patch was that it is critical that when a standalone application accesses global data, the version of the structure that it is accessing is exactly the same as the version being accessed by U-Boot. There are two ways of handling this:

1) Add an export function to access global data members by name (reporting an error if the member does not exist)
2) 'Sign' the structure of the global data (this patch)

Regards,

Graeme

Le 04/09/2011 01:50, Graeme Russ a ?crit :

Seems like the export function is what we should be aiming for, except that IMO the list of these functions should match a list of functional properties that U-Boot standalone applications need regardless to how these properties are computed, not a list of members that happen to exist in a structure at a given moment.

Amicalement,

Albert.

Hi Wolfgang,

[snip]

I know that this patch is essentially dead given the fact that standalone applications should not access gd directly, but a few points for future reference....

The CRC could be calculated just on the struct members, but my sed-fu is not that good. If it was done that way, the only way the CRC would change is by either:

1) Members being added
2) Members being removed
3) Members being moved around
4) Members type being changes(*)
5) Members being renamed

Comments and white spaces have no impact on the CRC as they are stripped. (*)For CRC calculateion, member types are reduced to their ANSI C equivalent unless typedef'd, so some member type changes will not trigger a CRC change

Now apart from #5, we would want a new CRC for any of those modifications, and how likely is #5 without some other change to gd along the way?

Not needed as long as only core U-Boot code accessed gd

Don't know - As I said in the commit message, this came about from idle curiosity about the relationship between gd and XF_VERSION

As an aside, maybe we could expose gd members to standalone applications via getenv - create psuedo environment variables that are actually tied to gd members? But then again, I really do not know if standalone applications *ever* use gd, but getenv may be a nice safe way of exporting gd to standalone applications

Regards,

Graeme