| Submitter | Mathias Krause |
|---|---|
| Date | 2011-01-28 09:03:27 |
| Message ID | <4D42865F.8030105@secunet.com> |
| Download | mbox | patch |
| Permalink | /patch/2572/ |
| State | Bitrotted |
| Headers | show |
Comments
On Fri, Jan 28, 2011 at 1:03 AM, Mathias Krause <mathias.krause@secunet.com>wrote: > Hi, > > flashrom currently tries to unlock the chip before each read/write/erase > command. To prevent further modifications of the chip content, e.g. for > security reasons, it would be helpful if flashrom would also be capable > of the opposite operation. I talked with Carl-Daniel a little about it > and he mentioned problems like having to implement features like partial > locks, chips that cannot be unlocked after being locked, etc. To support > those features and handle the problematic chips something more than this > little patch can do must be implemented. But for now it would be nice to > have *something*. See it as a starting point to solve the much bigger > problem. > Yep, it certainly gets hairy. On Fri, Jan 28, 2011 at 1:03 AM, Mathias Krause <mathias.krause@secunet.com> wrote: > This patch adds a function pointer to struct flashchip and a command > line option to trigger this function to enable full flash chip write > protection. It's currently only implemented for the Atmel AT26DF081A > since this is the chip I'm currently working with and which I can test > easily. > For what it's worth, we've been doing something similar in the Chromium OS branch (http://git.chromium.org/gitweb/?p=flashrom.git;a=tree). Apologies if it seems a bit sloppy at the moment, we've had to wedge in a lot stuff to solve immediate concerns. We also added a member to the flashchip structure (.wp), but in our case it is a pointer to a "writeprotect" structure which contains a bunch of function pointers. We felt this was necessary since write protection varies greatly between chips and we want fine-grained control. In our model, the user specifies the byte range (offset and length) and enables write protection in two steps. For example: "flashrom --wp-range 0x200000 0x200000 --wp-enable". Most of the ugly stuff in our code is contained within writeprotect.c. So if you want to give it a try it should integrate into your tree pretty easily.
On Fri, Jan 28, 2011 at 3:00 PM, David Hendricks <dhendrix@google.com>wrote: > On Fri, Jan 28, 2011 at 1:03 AM, Mathias Krause < > mathias.krause@secunet.com> wrote: > >> Hi, >> >> flashrom currently tries to unlock the chip before each read/write/erase >> command. To prevent further modifications of the chip content, e.g. for >> security reasons, it would be helpful if flashrom would also be capable >> of the opposite operation. I talked with Carl-Daniel a little about it >> and he mentioned problems like having to implement features like partial >> locks, chips that cannot be unlocked after being locked, etc. To support >> those features and handle the problematic chips something more than this >> little patch can do must be implemented. But for now it would be nice to >> have *something*. See it as a starting point to solve the much bigger >> problem. >> > > Yep, it certainly gets hairy. > > On Fri, Jan 28, 2011 at 1:03 AM, Mathias Krause < > mathias.krause@secunet.com> wrote: > >> This patch adds a function pointer to struct flashchip and a command >> line option to trigger this function to enable full flash chip write >> protection. It's currently only implemented for the Atmel AT26DF081A >> since this is the chip I'm currently working with and which I can test >> easily. >> > > For what it's worth, we've been doing something similar in the Chromium OS > branch (http://git.chromium.org/gitweb/?p=flashrom.git;a=tree). Apologies > if it seems a bit sloppy at the moment, we've had to wedge in a lot stuff to > solve immediate concerns. > > We also added a member to the flashchip structure (.wp), but in our case it > is a pointer to a "writeprotect" structure which contains a bunch of > function pointers. We felt this was necessary since write protection varies > greatly between chips and we want fine-grained control. > > In our model, the user specifies the byte range (offset and length) and > enables write protection in two steps. For example: "flashrom --wp-range > 0x200000 0x200000 --wp-enable". > > Most of the ugly stuff in our code is contained within writeprotect.c. So > if you want to give it a try it should integrate into your tree pretty > easily. > > -- > David Hendricks (dhendrix) > Systems Software Engineer, Google Inc. > It seems we keep missing each other on IRC, so let's just continue the discussion here. <minipli> dhendrix: I looked at the google flashrom write protect code and it actually does a different thing then my patch <minipli> dhendrix: it has the possibility to write protect (and after that, unprotect again) subranges of the flash memory <minipli> dhendrix: my patch just enables write protect for the whole chip and locks the register afterwards <minipli> dhendrix: the lock, though, is only as effective, as the WP pin indicates (either hardware locked or software locked) The Chromium OS code does it in two parts. First, the user specifies a range (--wp-range). Second, the user enables write protection (--wp-enable). As I understand, for write protection to take effect the WP pin must be active. Until that happens, the status register which holds the block protection status and WP enable bit can be modified by software. In any case, I believe the ends which you are trying to achieve can be accomplished using the Chromium OS approach. However, there are many machines in the field which require partial firmware updates, and thus cannot be compatible with the coarse-grained approach.
On 01.02.2011 20:41, David Hendricks wrote: > It seems we keep missing each other on IRC, so let's just continue the > discussion here. > > <minipli> dhendrix: I looked at the google flashrom write protect code and it actually does a different thing then my patch > <minipli> dhendrix: it has the possibility to write protect (and after that, unprotect again) subranges of the flash memory > <minipli> dhendrix: my patch just enables write protect for the whole chip and locks the register afterwards > <minipli> dhendrix: the lock, though, is only as effective, as the WP pin indicates (either hardware locked or software locked) > > The Chromium OS code does it in two parts. First, the user specifies a > range (--wp-range). Second, the user enables write protection (--wp-enable). Actually that is a good thing. At least unexperienced users might want to check different ranges and see if it works for them, before locking the status register down. > As I understand, for write protection to take effect the WP pin must be > active. Until that happens, the status register which holds the block > protection status and WP enable bit can be modified by software. That's mostly true at least for the chip I'm currently looking at. The Atmel AT26DF081A has a status registers with the Sector Protection Register Lock bit (SPRL) indicating the lock state of the register - I guess any SPI chip has something similar to this. To write protect the whole chip a special value must be written into the status register to normally r/o fields (XX1111XX binary for "Global Protect"). After that the chip is write protected against "accidental write attempts." Write and erase commands send to the chip will simply fail. To make them succeed the chip must be unprotected first by disabling the global write protect. So far so good. For security and/or safety reasons one might want to write protect the whole chip and prevent undoing the write protect. That's what the SPRL is used for. If you set this bit to 1 the status register is locked and cannot be modified any further ... well, it depends ;) If the WP pin is connected to GND the lock is enforced by the SPI chip and can only be reset through a power cycle of the chip. (Keep in mind, S5 might power the SPI chip too, so disconnecting the AC connector may be needed to reset this register). If the WP pin is connected to Vcc the SPRL can be reset by software, effectively disabling the lock and in turn being able to disable the write protection. Not good from a security standpoint. So to actually write protect a chip the way one would expect it, hardware and software must fit together. > In any case, I believe the ends which you are trying to achieve can be > accomplished using the Chromium OS approach. However, there are many > machines in the field which require partial firmware updates, and thus > cannot be compatible with the coarse-grained approach. I see your point of having partial write protects to always being able to recover from a bad flash by having a fixed recovery part in the flash that won't be overwritten, but as you already mentioned it gets hairy. The structure you're proposing has nice features of printing the locking capabilities and checking the write protect status. Trying to match this to the capabilities of the AT26DF081A, I can just say, it doesn't seem to fit. (1) The internal sector layout - which must be used, when you want partial write protect - is rather scary (the layout looks like top to bottom: 32k, 2 x 8k, 16k, 15 x 64k). I don't know how to give this information nicely to the user so he can decide which address range is required to fit into the locking scheme of the chip. (2) Printing the write protect status is just not possible because no command to get this information does exist. The bits used to enable/disable the global protect/unprotect are used for something else when reading the status register. The protect/unprotect sector opcodes have no counterpart to read out the protection map. So to just skip all those problems for now I was proposing the simplest form of the write protection - just lock the whole chip. This seems to be easy for the AT26DF081A and should be so for other chips, too. Until we* have a better understanding of how other chips use to work, this scheme seems to be a start to match some user needs. From a user standpoint (the command line interface) this can be easily extended to support partial write protect later on, e.g. make -K take an optional argument for the address range(s) to protect. Greets, Mathias * Well, at least me, since I don't know much about how flash chips do operate yet.
On Wed, Feb 2, 2011 at 2:12 AM, Mathias Krause <mathias.krause@secunet.com>wrote: > On 01.02.2011 20:41, David Hendricks wrote: > > As I understand, for write protection to take effect the WP pin must be > > active. Until that happens, the status register which holds the block > > protection status and WP enable bit can be modified by software. > > That's mostly true at least for the chip I'm currently looking at. The > Atmel AT26DF081A has a status registers with the Sector Protection > Register Lock bit (SPRL) indicating the lock state of the register - I > guess any SPI chip has something similar to this. To write protect the > whole chip a special value must be written into the status register to > normally r/o fields (XX1111XX binary for "Global Protect"). After that > the chip is write protected against "accidental write attempts." Write > and erase commands send to the chip will simply fail. To make them > succeed the chip must be unprotected first by disabling the global write > protect. So far so good. > For security and/or safety reasons one might want to write protect the > whole chip and prevent undoing the write protect. That's what the SPRL > is used for. If you set this bit to 1 the status register is locked and > cannot be modified any further ... well, it depends ;) If the WP pin is > connected to GND the lock is enforced by the SPI chip and can only be > reset through a power cycle of the chip. (Keep in mind, S5 might power > the SPI chip too, so disconnecting the AC connector may be needed to > reset this register). If the WP pin is connected to Vcc the SPRL can be > reset by software, effectively disabling the lock and in turn being able > to disable the write protection. Not good from a security standpoint. > > So to actually write protect a chip the way one would expect it, > hardware and software must fit together. > That is correct. I'm just sayin' that our approaches have the same weakness :-) On Wed, Feb 2, 2011 at 2:12 AM, Mathias Krause <mathias.krause@secunet.com> wrote: > > In any case, I believe the ends which you are trying to achieve can be > > accomplished using the Chromium OS approach. However, there are many > > machines in the field which require partial firmware updates, and thus > > cannot be compatible with the coarse-grained approach. > > I see your point of having partial write protects to always being able > to recover from a bad flash by having a fixed recovery part in the flash > that won't be overwritten, but as you already mentioned it gets hairy. > The structure you're proposing has nice features of printing the locking > capabilities and checking the write protect status. Trying to match this > to the capabilities of the AT26DF081A, I can just say, it doesn't seem > to fit. I just opened up the datasheet for the AT26 part... yikes! I see now that these parts use a bunch of new opcodes rather than just changing status register bits. This is indeed dramatically different than the flash chips I was looking at (mostly Winbond and Macronix W25 and MX25 chips). On Wed, Feb 2, 2011 at 2:12 AM, Mathias Krause <mathias.krause@secunet.com> wrote: > (1) The internal sector layout - which must be used, when you > want partial write protect - is rather scary (the layout looks like top > to bottom: 32k, 2 x 8k, 16k, 15 x 64k). I don't know how to give this > information nicely to the user so he can decide which address range is > required to fit into the locking scheme of the chip. In the Chromium OS write protection code, we try to abstract details of the block layout and boil it down to a listing of valid ranges (starting offset and length). This is pretty easy with the table-driven approach when there are only 4-32 possible combinations. But as you correctly point out, this does not really work for the AT26. For example, if the user wants to block protect only the upper 64KB on the AT26DF081A, flashrom's write protection code would need to know to map that to the 32k, 2x 8k, and 16k sectors. Where as if the user only wants to write protect the bottom 64KB, flashrom only needs to map to a single sector. And of course there are a lot of invalid combinations as well. What a mess. I suppose if we were to use the Chromium OS approach for this chip then we could start by simply presenting the user with one valid option for --wp-range (start=0, length=0x100000) option which uses the "Protect Sector" commands on all 16 blocks. Then the --wp-enable command performs the "Write Disable" command. More intelligent sector handling would need to be added to deal with finer-grained locking on this chip. On Wed, Feb 2, 2011 at 2:12 AM, Mathias Krause <mathias.krause@secunet.com> wrote: > (2) Printing the > write protect status is just not possible because no command to get this > information does exist. The bits used to enable/disable the global > protect/unprotect are used for something else when reading the status > register. The protect/unprotect sector opcodes have no counterpart to > read out the protection map. > > So to just skip all those problems for now I was proposing the simplest > form of the write protection - just lock the whole chip. This seems to > be easy for the AT26DF081A and should be so for other chips, too. > > Until we* have a better understanding of how other chips use to work, > this scheme seems to be a start to match some user needs. From a user > standpoint (the command line interface) this can be easily extended to > support partial write protect later on, e.g. make -K take an optional > argument for the address range(s) to protect. > > > Greets, > Mathias > > * Well, at least me, since I don't know much about how flash chips do > operate yet. > Yes, this approach makes a lot more sense now. I'm still not sure what the best long-term approach is since my current focus is kind of narrow :-/
Patch
Index: flash.h =================================================================== --- flash.h (Revision 1256) +++ flash.h (Arbeitskopie) @@ -136,6 +136,7 @@ int (*printlock) (struct flashchip *flash); int (*unlock) (struct flashchip *flash); + int (*lock) (struct flashchip *flash); int (*write) (struct flashchip *flash, uint8_t *buf, int start, int len); int (*read) (struct flashchip *flash, uint8_t *buf, int start, int len); @@ -207,7 +208,7 @@ void print_banner(void); void list_programmers_linebreak(int startcol, int cols, int paren); int selfcheck(void); -int doit(struct flashchip *flash, int force, char *filename, int read_it, int write_it, int erase_it, int verify_it); +int doit(struct flashchip *flash, int force, char *filename, int read_it, int write_it, int erase_it, int verify_it, int lock_it); int read_buf_from_file(unsigned char *buf, unsigned long size, char *filename); int write_buf_to_file(unsigned char *buf, unsigned long size, char *filename); Index: cli_classic.c =================================================================== --- cli_classic.c (Revision 1256) +++ cli_classic.c (Arbeitskopie) @@ -37,7 +37,7 @@ #if CONFIG_PRINT_WIKI == 1 "-z|" #endif - "-E|-r <file>|-w <file>|-v <file>]\n" + "-E|-K|-r <file>|-w <file>|-v <file>]\n" " [-c <chipname>] [-m [<vendor>:]<part>] [-l <file>]\n" " [-i <image>] [-p <programmername>[:<parameters>]]\n\n"); @@ -57,6 +57,7 @@ " -v | --verify <file> verify flash against " "<file>\n" " -E | --erase erase flash device\n" + " -K | --lock lock flash device\n" " -V | --verbose more verbose output\n" " -c | --chip <chipname> probe only for specified " "flash chip\n" @@ -85,7 +86,7 @@ #if CONFIG_PRINT_WIKI == 1 "-z, " #endif - "-E, -r, -w, -v or no operation.\n" + "-E, -K, -r, -w, -v or no operation.\n" "If no operation is specified, flashrom will only probe for " "flash chips.\n\n"); } @@ -106,7 +107,7 @@ int opt; int option_index = 0; int force = 0; - int read_it = 0, write_it = 0, erase_it = 0, verify_it = 0; + int read_it = 0, write_it = 0, erase_it = 0, verify_it = 0, lock_it = 0; int dont_verify_it = 0, list_supported = 0; #if CONFIG_PRINT_WIKI == 1 int list_supported_wiki = 0; @@ -114,11 +115,12 @@ int operation_specified = 0; int i; - static const char optstring[] = "r:Rw:v:nVEfc:m:l:i:p:Lzh"; + static const char optstring[] = "r:Rw:v:nVEKfc:m:l:i:p:Lzh"; static const struct option long_options[] = { {"read", 1, NULL, 'r'}, {"write", 1, NULL, 'w'}, {"erase", 0, NULL, 'E'}, + {"lock", 0, NULL, 'K'}, {"verify", 1, NULL, 'v'}, {"noverify", 0, NULL, 'n'}, {"chip", 1, NULL, 'c'}, @@ -208,6 +210,14 @@ } erase_it = 1; break; + case 'K': + if (++operation_specified > 1) { + fprintf(stderr, "More than one operation " + "specified. Aborting.\n"); + cli_classic_abort_usage(); + } + lock_it = 1; + break; case 'm': #if CONFIG_INTERNAL == 1 tempstr = strdup(optarg); @@ -409,14 +419,14 @@ return 1; } - if (!(read_it | write_it | verify_it | erase_it)) { + if (!(read_it | write_it | verify_it | erase_it | lock_it)) { printf("No operations were specified.\n"); // FIXME: flash writes stay enabled! programmer_shutdown(); exit(0); } - if (!filename && !erase_it) { + if (!filename && !(erase_it || lock_it)) { printf("Error: No filename specified.\n"); // FIXME: flash writes stay enabled! programmer_shutdown(); @@ -432,5 +442,5 @@ * Give the chip time to settle. */ programmer_delay(100000); - return doit(flash, force, filename, read_it, write_it, erase_it, verify_it); + return doit(flash, force, filename, read_it, write_it, erase_it, verify_it, lock_it); } Index: spi25.c =================================================================== --- spi25.c (Revision 1256) +++ spi25.c (Arbeitskopie) @@ -1197,6 +1197,44 @@ return 0; } +int spi_enable_blockprotect_at25df(struct flashchip *flash) +{ + uint8_t status; + int result; + + /* Reset SPRL when needed to be able to modify status register */ + status = spi_read_status_register(); + if (status & (1 << 7)) { + msg_cdbg("Need to disable Sector Protection Register Lock\n"); + if ((status & (1 << 4)) == 0) { + msg_cerr("WP# pin is active, disabling " + "write protection is impossible.\n"); + return 1; + } + /* All bits except bit 7 (SPRL) are readonly. */ + result = spi_write_status_register(flash, status & ~(1 << 7)); + if (result) { + msg_cerr("spi_write_status_register failed\n"); + return result; + } + + } + /* Global protect. Make sure to set SPRL as well. */ + result = spi_write_status_register(flash, status | 0xbc); + if (result) { + msg_cerr("spi_write_status_register failed\n"); + return result; + } + status = spi_read_status_register(); + if ((status & (3 << 2)) != (3 << 2)) { + msg_cerr("Block protection could not be enabled!\n"); + return 1; + } + msg_cinfo("Block protection enabled, %s locked!\n", + (status & (1 << 4)) ? "software" : "hardware"); + return 0; +} + int spi_nbyte_read(int address, uint8_t *bytes, int len) { const unsigned char cmd[JEDEC_READ_OUTSIZE] = { Index: flashchips.c =================================================================== --- flashchips.c (Revision 1256) +++ flashchips.c (Arbeitskopie) @@ -1501,6 +1501,7 @@ } }, .unlock = spi_disable_blockprotect_at25df, + .lock = spi_enable_blockprotect_at25df, .write = spi_chip_write_256, .read = spi_chip_read, }, Index: flashrom.c =================================================================== --- flashrom.c (Revision 1256) +++ flashrom.c (Arbeitskopie) @@ -1759,7 +1759,7 @@ /* FIXME: This function signature needs to be improved once doit() has a better * function signature. */ -int chip_safety_check(struct flashchip *flash, int force, char *filename, int read_it, int write_it, int erase_it, int verify_it) +int chip_safety_check(struct flashchip *flash, int force, char *filename, int read_it, int write_it, int erase_it, int verify_it, int lock_it) { if (!programmer_may_write && (write_it || erase_it)) { msg_perr("Write/erase is not working yet on your programmer in " @@ -1809,6 +1809,13 @@ return 1; } } + if (lock_it) { + if (!flash->lock) { + msg_cerr("flashrom has no lock function for this " + "flash chip.\n"); + return 1; + } + } return 0; } @@ -1816,14 +1823,14 @@ * but right now it allows us to split off the CLI code. * Besides that, the function itself is a textbook example of abysmal code flow. */ -int doit(struct flashchip *flash, int force, char *filename, int read_it, int write_it, int erase_it, int verify_it) +int doit(struct flashchip *flash, int force, char *filename, int read_it, int write_it, int erase_it, int verify_it, int lock_it) { uint8_t *oldcontents; uint8_t *newcontents; int ret = 0; unsigned long size = flash->total_size * 1024; - if (chip_safety_check(flash, force, filename, read_it, write_it, erase_it, verify_it)) { + if (chip_safety_check(flash, force, filename, read_it, write_it, erase_it, verify_it, lock_it)) { msg_cerr("Aborting.\n"); ret = 1; goto out_nofree; @@ -1835,6 +1842,11 @@ if (flash->unlock) flash->unlock(flash); + if (lock_it) { + ret = flash->lock(flash); + goto out_nofree; + } + if (read_it) { ret = read_flash_to_file(flash, filename); goto out_nofree; Index: chipdrivers.h =================================================================== --- chipdrivers.h (Revision 1256) +++ chipdrivers.h (Arbeitskopie) @@ -54,6 +54,7 @@ int spi_disable_blockprotect_at25f(struct flashchip *flash); int spi_disable_blockprotect_at25fs010(struct flashchip *flash); int spi_disable_blockprotect_at25fs040(struct flashchip *flash); +int spi_enable_blockprotect_at25df(struct flashchip *flash); int spi_byte_program(int addr, uint8_t databyte); int spi_nbyte_program(int addr, uint8_t *bytes, int len); int spi_nbyte_read(int addr, uint8_t *bytes, int len);
Hi, flashrom currently tries to unlock the chip before each read/write/erase command. To prevent further modifications of the chip content, e.g. for security reasons, it would be helpful if flashrom would also be capable of the opposite operation. I talked with Carl-Daniel a little about it and he mentioned problems like having to implement features like partial locks, chips that cannot be unlocked after being locked, etc. To support those features and handle the problematic chips something more than this little patch can do must be implemented. But for now it would be nice to have *something*. See it as a starting point to solve the much bigger problem. This patch adds a function pointer to struct flashchip and a command line option to trigger this function to enable full flash chip write protection. It's currently only implemented for the Atmel AT26DF081A since this is the chip I'm currently working with and which I can test easily. ToDo: Add man page documentation. Signed-off-by: Mathias Krause <mathias.krause@secunet.com>