| Message ID | 1315048903-23217-1-git-send-email-graeme.russ@gmail.com |
|---|---|
| State | Rejected |
| Headers | show |
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
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
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
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
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
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,
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; +} + +
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