Patchwork [U-Boot,RFC] Generate CRC of structure of Global Data for standalone apps

login
register
mail settings
Submitter Graeme Russ
Date Sept. 3, 2011, 11:21 a.m.
Message ID <1315048903-23217-1-git-send-email-graeme.russ@gmail.com>
Download mbox | patch
Permalink /patch/113226/
State Rejected
Headers show

Comments

Graeme Russ - Sept. 3, 2011, 11:21 a.m.
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

--
1.7.5.2.317.g391b14
Wolfgang Denk - Sept. 3, 2011, 12:13 p.m.
Dear Graeme Russ,

In message <1315048903-23217-1-git-send-email-graeme.russ@gmail.com> you wrote:
> 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'

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
Graeme Russ - Sept. 3, 2011, 12:32 p.m.
Hi Wolfgang,

On 03/09/11 22:13, Wolfgang Denk wrote:
> Dear Graeme Russ,
> 
> In message <1315048903-23217-1-git-send-email-graeme.russ@gmail.com> you wrote:
>> 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'
> 
> 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?

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
Wolfgang Denk - Sept. 3, 2011, 7:44 p.m.
Dear Graeme Russ,

In message <4E621E4E.8010703@gmail.com> you wrote:
> 
> 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?

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
Graeme Russ - Sept. 3, 2011, 11:50 p.m.
On 04/09/11 05:44, Wolfgang Denk wrote:
> Dear Graeme Russ,
> 
> In message <4E621E4E.8010703@gmail.com> you wrote:
>>
>> 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?
> 
> 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.

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
Graeme Russ - Sept. 4, 2011, 5:56 a.m.
Hi Wolfgang,

On 03/09/11 22:13, Wolfgang Denk wrote:
> Dear Graeme Russ,
> 
> In message <1315048903-23217-1-git-send-email-graeme.russ@gmail.com> you wrote:

[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 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'
> 
> 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.

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?

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

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

> 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?

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
Albert ARIBAUD - Sept. 4, 2011, 6:30 a.m.
Le 04/09/2011 01:50, Graeme Russ a écrit :
> On 04/09/11 05:44, Wolfgang Denk wrote:
>> Dear Graeme Russ,
>>
>> In message<4E621E4E.8010703@gmail.com>  you wrote:
>>>
>>> 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?
>>
>> 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.
>
> 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)

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.

> Regards,
>
> Graeme

Amicalement,

Patch

diff --git a/Makefile b/Makefile
index 049b169..df61b02 100644
--- a/Makefile
+++ b/Makefile
@@ -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 \
 	,@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 \
diff --git a/common/exports.c b/common/exports.c
index 717e4af..8e767cc 100644
--- a/common/exports.c
+++ b/common/exports.c
@@ -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;

diff --git a/examples/standalone/hello_world.c b/examples/standalone/hello_world.c
index 067c390..18e3a51 100644
--- a/examples/standalone/hello_world.c
+++ b/examples/standalone/hello_world.c
@@ -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);
diff --git a/include/_exports.h b/include/_exports.h
index 349a3c5..6ffa7b5 100644
--- a/include/_exports.h
+++ b/include/_exports.h
@@ -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)
diff --git a/include/exports.h b/include/exports.h
index 9492566..fc11a18 100644
--- a/include/exports.h
+++ b/include/exports.h
@@ -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;
diff --git a/tools/gencrc32header/Makefile b/tools/gencrc32header/Makefile
new file mode 100644
index 0000000..612d872
--- /dev/null
+++ b/tools/gencrc32header/Makefile
@@ -0,0 +1,62 @@ 
+# (C) Copyright 20011
+# Graeme Russ, <graeme.russ@gmail.com>
+#
+# See file CREDITS for list of people who contributed to this
+# project.
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License as
+# published by the Free Software Foundation; either version 2 of
+# the License, or (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 59 Temple Place, Suite 330, Boston,
+# MA 02111-1307 USA
+
+include $(TOPDIR)/config.mk
+
+# Generated executable files
+BIN_FILES-y += gencrc32header
+
+# Source files which exist outside the tools/gencrc32header directory
+EXT_OBJ_FILES-y += lib/crc32.o
+
+# Source files located in the tools/gencrc32header directory
+OBJ_FILES-y += gencrc32header.o
+
+# now $(obj) is defined
+SRCS	+= $(addprefix $(SRCTREE)/,$(EXT_OBJ_FILES-y:.o=.c))
+SRCS	+= $(addprefix $(SRCTREE)/tools/,$(OBJ_FILES-y:.o=.c))
+BINS	:= $(addprefix $(obj),$(sort $(BIN_FILES-y)))
+
+all:	$(BINS)
+
+$(obj)gencrc32header:	$(obj)gencrc32header.o $(obj)crc32.o
+	$(CC) $(HOSTCFLAGS) $(HOSTLDFLAGS) -o $@ $^
+	$(STRIP) $@
+
+## Some of the tool objects need to be accessed from outside the tools/gencrc32header directory
+$(obj)%.o: %.c
+	$(CC) -g $(HOSTCFLAGS) -c -o $@ $<
+
+$(obj)%.o: $(SRCTREE)/lib/%.c
+	$(CC) -g -DUSE_HOSTCC -I $(SRCTREE)/include $(HOSTCFLAGS) -c -o $@ $<
+
+
+clean:
+	rm -rf *.o gencrc32header
+
+#########################################################################
+
+# defines $(obj).depend target
+include $(SRCTREE)/rules.mk
+
+sinclude $(obj).depend
+
+#########################################################################
diff --git a/tools/gencrc32header/gencrc32header.c b/tools/gencrc32header/gencrc32header.c
new file mode 100644
index 0000000..ce82684
--- /dev/null
+++ b/tools/gencrc32header/gencrc32header.c
@@ -0,0 +1,80 @@ 
+/*
+ * (C) Copyright 20011
+ * Graeme Russ, <graeme.russ@gmail.com>
+ *
+ * See file CREDITS for list of people who contributed to this
+ * project.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 of
+ * the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
+ * MA 02111-1307 USA
+ */
+
+#include <errno.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <stddef.h>
+#include <string.h>
+#include <sys/types.h>
+#include <sys/ioctl.h>
+#include <sys/stat.h>
+#include <unistd.h>
+
+#define BUFFER_GRANULE_SIZE	256
+extern unsigned long crc32(unsigned long crc, const char *buf, unsigned int len);
+
+int main (int argc, char **argv)
+{
+	char *buf = NULL;
+	long byte_count = 0;
+	long buf_len = 0;
+	unsigned long crc;
+
+	int in_char;
+
+	buf_len = BUFFER_GRANULE_SIZE;
+	buf = malloc(buf_len);
+
+	while (buf && ((in_char = fgetc(stdin)) != EOF)) {
+		if (byte_count >= buf_len) {
+			buf_len += BUFFER_GRANULE_SIZE;
+			buf = realloc(buf, buf_len);
+		}
+		if (buf)
+			buf[byte_count++] = (char)in_char;
+	}
+
+	if (buf) {
+		crc = crc32(0, buf, byte_count);
+
+		printf("#ifndef __GLOBAL_DATA_CRC_H__\n");
+		printf("#define __GLOBAL_DATA_CRC_H__\n");
+		printf("/*\n");
+		printf(" * DO NOT MODIFY.\n");
+		printf(" *\n");
+		printf(" * This file was auto-generated\n");
+		printf(" *\n");
+		printf(" */\n");
+		printf("\n");
+		printf("#define XF_GLOBAL_DATA_CRC\t0x%8.8lxUL\n", crc);
+		printf("\n");
+		printf("#endif\n");
+
+		return 0;
+	}
+
+	return -1;
+}
+
+