flashrom January 2014

flashrom@flashrom.org

28 participants
73 discussions

[PATCH] CID1130008: Resource leak
by Stefan Reinauer 26 Apr '14

26 Apr '14

CID1130008: Resource leak The system resource will not be reclaimed and reused, reducing the future availability of the resource. In read_romlayout: Leak of memory or pointers to system resources Signed-off-by: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Index: layout.c =================================================================== --- layout.c (revision 1763) +++ layout.c (working copy) @@ -65,6 +65,7 @@ if (num_rom_entries >= MAX_ROMLAYOUT) { msg_gerr("Maximum number of ROM images (%i) in layout " "file reached.\n", MAX_ROMLAYOUT); + fclose(romlayout); return 1; } if (2 != fscanf(romlayout, "%s %s\n", tempstr, rom_entries[num_rom_entries].name))

2 1

[PATCH] CID1130007: Resource leak
by Stefan Reinauer 26 Apr '14

26 Apr '14

CID1130007: Resource leak The system resource will not be reclaimed and reused, reducing the future availability of the resource. In ogp_spi_init: Leak of memory or pointers to system resources Signed-off-by: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Index: ogp_spi.c =================================================================== --- ogp_spi.c (revision 1763) +++ ogp_spi.c (working copy) @@ -120,8 +120,10 @@ ogp_reg_sck = OGA1_XP10_CPROM_SCK; } else { msg_perr("Invalid or missing rom= parameter.\n"); + free(type); return 1; } + free(type); if (rget_io_perms()) return 1;

2 1

[PATCH] CID1130011: Use after free
by Stefan Reinauer 26 Apr '14

26 Apr '14

CID1130011: Use after free This could cause an immediate crash or incorrect values might be read subsequently resulting in incorrect computations. In dump_file: A pointer to freed memory is dereferenced, used as a function argument, or otherwise used Signed-off-by: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Index: util/ich_descriptors_tool/ich_descriptors_tool.c =================================================================== --- util/ich_descriptors_tool/ich_descriptors_tool.c (revision 1763) +++ util/ich_descriptors_tool/ich_descriptors_tool.c (working copy) @@ -77,12 +77,13 @@ printf("Dumping %u bytes of the %s region from 0x%08x-0x%08x to %s... ", file_len, region_names[i], base, limit, fn); int fh = open(fn, O_WRONLY | O_CREAT, S_IRUSR | S_IWUSR); - free(fn); if (fh < 0) { fprintf(stderr, "ERROR: couldn't open(%s): %s\n", fn, strerror(errno)); + free(fn); exit(1); } + free(fn); ret = write(fh, &dump[base >> 2], file_len); if (ret != file_len) {

2 1

[PATCH] CID1130012: Double free
by Stefan Reinauer 26 Apr '14

26 Apr '14

CID1130012: Double free Depending on the implementation of the deallocator function, the memory may by placed on the free list more than once. In pony_spi_init: Memory is deallocated more than once Signed-off-by: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Index: pony_spi.c =================================================================== --- pony_spi.c (revision 1763) +++ pony_spi.c (working copy) @@ -140,6 +140,7 @@ } else if (arg && !strlen(arg)) { msg_perr("Error: Missing argument for programmer type.\n"); free(arg); + return 1; } else if (arg){ msg_perr("Error: Invalid programmer type specified.\n"); free(arg);

2 1

[PATCH] CID1130004: Nesting level does not match indentation
by Stefan Reinauer 26 Apr '14

26 Apr '14

CID1130004: Nesting level does not match indentation Code that is meant to be executed conditionally may be executed unconditionally In print_supported_boards_helper: The indentation of this code suggests it is nested when it is not. Signed-off-by: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Index: print.c =================================================================== --- print.c (revision 1763) +++ print.c (working copy) @@ -436,13 +436,14 @@ msg_ginfo("%s", b->name); for (i = 0; i < maxboardlen - strlen(b->name); i++) msg_ginfo(" "); - if (b->working == OK) - msg_ginfo("OK "); - else if (b->working == NT) - msg_ginfo("NT "); - else - msg_ginfo("BAD "); + if (b->working == OK) + msg_ginfo("OK "); + else if (b->working == NT) + msg_ginfo("NT "); + else + msg_ginfo("BAD "); + for (e = board_matches; e->vendor_name != NULL; e++) { if (strcmp(e->vendor_name, b->vendor) || strcmp(e->board_name, b->name))

2 1

[PATCH] CID1129996: Unchecked return value from library
by Stefan Reinauer 26 Apr '14

26 Apr '14

CID1129996: Unchecked return value from library The function returns a value that indicates an error condition. If this is not checked, the error condition may not be handled correctly. In sp_opensocket: Value returned from a library function is not checked for errors before being used. This value may indicate an error condition. (CWE-252) Signed-off-by: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Index: serprog.c =================================================================== --- serprog.c (revision 1763) +++ serprog.c (working copy) @@ -114,7 +114,11 @@ } /* We are latency limited, and sometimes do write-write-read * * (write-n) - so enable TCP_NODELAY. */ - setsockopt(sock, IPPROTO_TCP, TCP_NODELAY, &flag, sizeof(int)); + if (setsockopt(sock, IPPROTO_TCP, TCP_NODELAY, &flag, sizeof(int))) { + close(sock); + msg_perr("Error: serprog cannot set socket options: %s\n", strerror(errno)); + return -1; + } return sock; } #endif

2 1

[PATCH] CID1130000: Unchecked return value
by Stefan Reinauer 26 Apr '14

26 Apr '14

CID1130000: Unchecked return value If the function returns an error value, the error value may be mistaken for a normal value. In default_spi_write_aai: Value returned from a function is not checked for errors before being used Signed-off-by: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Index: spi25.c =================================================================== --- spi25.c (revision 1763) +++ spi25.c (working copy) @@ -1146,8 +1146,10 @@ while (pos < start + len - 1) { cmd[1] = buf[pos++ - start]; cmd[2] = buf[pos++ - start]; - spi_send_command(flash, JEDEC_AAI_WORD_PROGRAM_CONT_OUTSIZE, 0, - cmd, NULL); + result = spi_send_command(flash, JEDEC_AAI_WORD_PROGRAM_CONT_OUTSIZE, + 0, cmd, NULL); + if (result != 0) + msg_cerr("%s failed.\n", __func__); while (spi_read_status_register(flash) & SPI_SR_WIP) programmer_delay(10); }

2 1

[PATCH] CID1130006: Resource leak
by Stefan Reinauer 26 Apr '14

26 Apr '14

CID1130006: Resource leak The system resource will not be reclaimed and reused, reducing the future availability of the resource. In buspirate_spi_init: Leak of memory or pointers to system resources Signed-off-by: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Index: buspirate_spi.c =================================================================== --- buspirate_spi.c (revision 1763) +++ buspirate_spi.c (working copy) @@ -250,6 +250,7 @@ if (!bp_commbuf) { bp_commbufsize = 0; msg_perr("Out of memory!\n"); + free(dev); return ERROR_OOM; } bp_commbufsize = DEFAULT_BUFSIZE;

2 1

[PATCH] CID1130002: Logically dead code
by Stefan Reinauer 26 Apr '14

26 Apr '14

CID1130002: Logically dead code The indicated dead code may have performed some action; that action will never occur. In satamv_init: Code can never be reached because of a logical contradiction What we really want to do is check whether PCI_BASE_ADDRESS_2 is valid at this point, and not PCI_BASE_ADDRESS_0. Signed-off-by: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Index: satamv.c =================================================================== --- satamv.c (revision 1763) +++ satamv.c (working copy) @@ -139,7 +139,7 @@ /* Get I/O BAR location. */ tmp = pcidev_readbar(dev, PCI_BASE_ADDRESS_2); - if (!addr) + if (!tmp) return 1; /* Truncate to reachable range.

2 1

[PATCH] Refactor spi_read_status_register usage
by Carl-Daniel Hailfinger 08 Mar '14

08 Mar '14

spi_read_status_register() is used in open-coded loops everywhere just to check if SPI_SR_WIP bit cleared. The logic is missing a timeout detection, and we can save quite a lot of code by introducing a function which waits until SPI_SR_WIP is cleared or a timeout is reached. Untested. May explode. Unfinished. More functions need to be converted to the new style. Will change behaviour if the chip is too slow or hangs. Should prevent flashrom from hanging without any visible output. Signed-off-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006(a)gmx.net> Index: flashrom-spi_rdsr_refactor/spi25.c =================================================================== --- flashrom-spi_rdsr_refactor/spi25.c (Revision 1653) +++ flashrom-spi_rdsr_refactor/spi25.c (Arbeitskopie) @@ -328,14 +328,9 @@ __func__); return result; } - /* Wait until the Write-In-Progress bit is cleared. - * This usually takes 1-85 s, so wait in 1 s steps. - */ - /* FIXME: We assume spi_read_status_register will never fail. */ - while (spi_read_status_register(flash) & SPI_SR_WIP) - programmer_delay(1000 * 1000); /* FIXME: Check the status register for errors. */ - return 0; + result = spi_wait_status_register_ready(flash, 120 * 1000 * 1000, JEDEC_CE_60); + return result; } int spi_chip_erase_62(struct flashctx *flash) @@ -365,14 +360,9 @@ __func__); return result; } - /* Wait until the Write-In-Progress bit is cleared. - * This usually takes 2-5 s, so wait in 100 ms steps. - */ - /* FIXME: We assume spi_read_status_register will never fail. */ - while (spi_read_status_register(flash) & SPI_SR_WIP) - programmer_delay(100 * 1000); /* FIXME: Check the status register for errors. */ - return 0; + result = spi_wait_status_register_ready(flash, 120 * 1000 * 1000, JEDEC_CE_62); + return result; } int spi_chip_erase_c7(struct flashctx *flash) @@ -401,14 +391,9 @@ msg_cerr("%s failed during command execution\n", __func__); return result; } - /* Wait until the Write-In-Progress bit is cleared. - * This usually takes 1-85 s, so wait in 1 s steps. - */ - /* FIXME: We assume spi_read_status_register will never fail. */ - while (spi_read_status_register(flash) & SPI_SR_WIP) - programmer_delay(1000 * 1000); /* FIXME: Check the status register for errors. */ - return 0; + result = spi_wait_status_register_ready(flash, 120 * 1000 * 1000, JEDEC_CE_C7); + return result; } int spi_block_erase_52(struct flashctx *flash, unsigned int addr, @@ -444,13 +429,9 @@ __func__, addr); return result; } - /* Wait until the Write-In-Progress bit is cleared. - * This usually takes 100-4000 ms, so wait in 100 ms steps. - */ - while (spi_read_status_register(flash) & SPI_SR_WIP) - programmer_delay(100 * 1000); /* FIXME: Check the status register for errors. */ - return 0; + result = spi_wait_status_register_ready(flash, 10 * 1000 * 1000, JEDEC_BE_52); + return result; } /* Block size is usually @@ -491,13 +472,9 @@ __func__, addr); return result; } - /* Wait until the Write-In-Progress bit is cleared. - * This usually takes 100-4000 ms, so wait in 100 ms steps. - */ - while (spi_read_status_register(flash) & SPI_SR_WIP) - programmer_delay(100 * 1000); /* FIXME: Check the status register for errors. */ - return 0; + result = spi_wait_status_register_ready(flash, 10 * 1000 * 1000, JEDEC_BE_D8); + return result; } /* Block size is usually @@ -536,13 +513,9 @@ __func__, addr); return result; } - /* Wait until the Write-In-Progress bit is cleared. - * This usually takes 100-4000 ms, so wait in 100 ms steps. - */ - while (spi_read_status_register(flash) & SPI_SR_WIP) - programmer_delay(100 * 1000); /* FIXME: Check the status register for errors. */ - return 0; + result = spi_wait_status_register_ready(flash, 10 * 1000 * 1000, JEDEC_BE_D7); + return result; } /* Sector size is usually 4k, though Macronix eliteflash has 64k */ @@ -579,13 +552,9 @@ __func__, addr); return result; } - /* Wait until the Write-In-Progress bit is cleared. - * This usually takes 15-800 ms, so wait in 10 ms steps. - */ - while (spi_read_status_register(flash) & SPI_SR_WIP) - programmer_delay(10 * 1000); /* FIXME: Check the status register for errors. */ - return 0; + result = spi_wait_status_register_ready(flash, 10 * 1000 * 1000, JEDEC_SE); + return result; } int spi_block_erase_50(struct flashctx *flash, unsigned int addr, unsigned int blocklen) @@ -619,13 +588,9 @@ msg_cerr("%s failed during command execution at address 0x%x\n", __func__, addr); return result; } - /* Wait until the Write-In-Progress bit is cleared. - * This usually takes 10 ms, so wait in 1 ms steps. - */ - while (spi_read_status_register(flash) & SPI_SR_WIP) - programmer_delay(1 * 1000); /* FIXME: Check the status register for errors. */ - return 0; + result = spi_wait_status_register_ready(flash, 10 * 1000 * 1000, JEDEC_BE_50); + return result; } int spi_block_erase_81(struct flashctx *flash, unsigned int addr, unsigned int blocklen) @@ -659,13 +624,9 @@ msg_cerr("%s failed during command execution at address 0x%x\n", __func__, addr); return result; } - /* Wait until the Write-In-Progress bit is cleared. - * This usually takes 8 ms, so wait in 1 ms steps. - */ - while (spi_read_status_register(flash) & SPI_SR_WIP) - programmer_delay(1 * 1000); /* FIXME: Check the status register for errors. */ - return 0; + result = spi_wait_status_register_ready(flash, 10 * 1000 * 1000, JEDEC_BE_81); + return result; } int spi_block_erase_60(struct flashctx *flash, unsigned int addr, Index: flashrom-spi_rdsr_refactor/spi25_statusreg.c =================================================================== --- flashrom-spi_rdsr_refactor/spi25_statusreg.c (Revision 1653) +++ flashrom-spi_rdsr_refactor/spi25_statusreg.c (Arbeitskopie) @@ -43,7 +43,6 @@ static int spi_write_status_register_flag(struct flashctx *flash, int status, const unsigned char enable_opcode) { int result; - int i = 0; /* * WRSR requires either EWSR or WREN depending on chip type. * The code below relies on the fact hat EWSR and WREN have the same @@ -78,17 +77,10 @@ /* WRSR performs a self-timed erase before the changes take effect. * This may take 50-85 ms in most cases, and some chips apparently * allow running RDSR only once. Therefore pick an initial delay of - * 100 ms, then wait in 10 ms steps until a total of 5 s have elapsed. + * 100 ms, then wait until a total of 5 s have elapsed. */ programmer_delay(100 * 1000); - while (spi_read_status_register(flash) & SPI_SR_WIP) { - if (++i > 490) { - msg_cerr("Error: WIP bit after WRSR never cleared\n"); - return TIMEOUT_ERROR; - } - programmer_delay(10 * 1000); - } - return 0; + return spi_wait_status_register_ready(flash, 4900 * 1000, JEDEC_WRSR); } int spi_write_status_register(struct flashctx *flash, int status) @@ -108,6 +100,28 @@ return ret; } +/* timeout is specified in usecs. */ +int spi_wait_status_register_ready(struct flashctx *flash, int timeout, uint8_t opcode) +{ + /* At least 1 usec between iterations. */ + int single_delay = (timeout / 100) ? : 1; + int elapsed = 0; + int halftime = 0; + + while (spi_read_status_register(flash) & SPI_SR_WIP) { + elapsed += single_delay; + if ((elapsed > timeout / 2) && !halftime) { + msg_cdbg("Debug: WIP bit after %02x didn't clear within %i us.\n", opcode, timeout/2); + } + if (elapsed >= timeout) { + msg_cerr("Error: WIP bit after %02x didn't clear within %i us.\n", opcode, timeout); + return TIMEOUT_ERROR; + } + programmer_delay(single_delay); + } + return 0; +} + uint8_t spi_read_status_register(struct flashctx *flash) { static const unsigned char cmd[JEDEC_RDSR_OUTSIZE] = { JEDEC_RDSR }; Index: flashrom-spi_rdsr_refactor/chipdrivers.h =================================================================== --- flashrom-spi_rdsr_refactor/chipdrivers.h (Revision 1653) +++ flashrom-spi_rdsr_refactor/chipdrivers.h (Arbeitskopie) @@ -60,6 +60,7 @@ /* spi25_statusreg.c */ uint8_t spi_read_status_register(struct flashctx *flash); +int spi_wait_status_register_ready(struct flashctx *flash, int timeout, uint8_t opcode); int spi_write_status_register(struct flashctx *flash, int status); int spi_prettyprint_status_register_plain(struct flashctx *flash); int spi_prettyprint_status_register_default_bp1(struct flashctx *flash); -- http://www.hailfinger.org/

1 1

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

2010

2009

flashrom January 2014