libflash/blocklevel: Make read/write be ECC agnostic for callers

The blocklevel abstraction allows for regions of the backing store to be
marked as ECC protected so that blocklevel can decode/encode the ECC
bytes into the buffer automatically without the caller having to be ECC
aware.

Unfortunately this abstraction is far from perfect, this is only useful
if reads and writes are performed at the start of the ECC region or in
some circumstances at an ECC aligned position - which requires the
caller be aware of the ECC regions.

The problem that has arisen is that the blocklevel abstraction is
initialised somewhere but when it is later called the caller is unaware
if ECC exists in the region it wants to arbitrarily read and write to.
This should not have been a problem since blocklevel knows. Currently
misaligned reads will fail ECC checks and misaligned writes will
overwrite ECC bytes and the backing store will become corrupted.

This patch add the smarts to blocklevel_read() and blocklevel_write() to
cope with the problem. Note that ECC can always be bypassed by calling
blocklevel_raw_() functions.

All this work means that the gard tool can can safely call
blocklevel_read() and blocklevel_write() and as long as the blocklevel
knows of the presence of ECC then it will deal with all cases.

This also commit removes code in the gard tool which compensated for
inadequacies no longer present in blocklevel.

Signed-off-by: Cyril Bur <cyril.bur@au1.ibm.com>
Tested-by: Pridhiviraj Paidipeddi <ppaidipe@linux.vnet.ibm.com>
[stewart: core/flash: Adapt to new libflash ECC API
Signed-off-by: Stewart Smith <stewart@linux.ibm.com>

core/flash.c

@@ -1,4 +1,4 @@
-/* Copyright 2013-2014 IBM Corp.
+/* Copyright 2013-2018 IBM Corp.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -683,8 +683,7 @@ static int flash_load_resource(enum resource_id id, uint32_t subid,
 	prlog(PR_DEBUG,"FLASH: %s partition %s ECC\n",
 	      name, ecc  ? "has" : "doesn't have");
 
-	if ((ecc ? ecc_buffer_size_minus_ecc(ffs_part_size) : ffs_part_size) <
-	     SECURE_BOOT_HEADERS_SIZE) {
+	if (ffs_part_size < SECURE_BOOT_HEADERS_SIZE) {
 		prerror("FLASH: secboot headers bigger than "
 			"partition size 0x%x\n", ffs_part_size);
 		goto out_free_ffs;
@@ -722,8 +721,6 @@ static int flash_load_resource(enum resource_id id, uint32_t subid,
 		}
 
 		ffs_part_start += SECURE_BOOT_HEADERS_SIZE;
-		if (ecc)
-			ffs_part_start += ecc_size(SECURE_BOOT_HEADERS_SIZE);
 
 		rc = blocklevel_read(flash->bl, ffs_part_start, bufp,
 					  content_size);

external/gard/gard.c

@@ -1,4 +1,4 @@
-/* Copyright 2013-2015 IBM Corp.
+/* Copyright 2013-2017 IBM Corp.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -36,7 +36,6 @@
 #include <libflash/libflash.h>
 #include <libflash/libffs.h>
 #include <libflash/file.h>
-#include <libflash/ecc.h>
 #include <libflash/blocklevel.h>
 #include <common/arch_flash.h>
 
@@ -58,7 +57,6 @@ extern const char version[];
 #define __unused __attribute__((unused))
 
 struct gard_ctx {
-	bool ecc;
 	uint32_t f_size;
 	uint32_t f_pos;
 
@@ -70,15 +68,6 @@ struct gard_ctx {
 	struct ffs_handle *ffs;
 };
 
-/*
- * Return the size of a struct gard_ctx depending on if the buffer contains
- * ECC bits
- */
-static inline size_t sizeof_gard(struct gard_ctx *ctx)
-{
-	return ctx->ecc ? ecc_buffer_size(sizeof(struct gard_record)) : sizeof(struct gard_record);
-}
-
 static void show_flash_err(int rc)
 {
 	switch (rc) {
@@ -374,11 +363,11 @@ static int do_iterate(struct gard_ctx *ctx,
 	struct gard_record gard, null_gard;
 
 	memset(&null_gard, UINT_MAX, sizeof(gard));
-	for (i = 0; i * sizeof_gard(ctx) < ctx->gard_data_len && rc == 0; i++) {
+	for (i = 0; i * sizeof(gard) < ctx->gard_data_len && rc == 0; i++) {
 		memset(&gard, 0, sizeof(gard));
 
-		rc = blocklevel_read(ctx->bl, ctx->gard_data_pos +
-				(i * sizeof_gard(ctx)), &gard, sizeof(gard));
+		rc = blocklevel_read(ctx->bl, ctx->gard_data_pos + (i * sizeof(gard)),
+					&gard, sizeof(gard));
 		/* It isn't super clear what constitutes the end, this should do */
 		if (rc || memcmp(&gard, &null_gard, sizeof(gard)) == 0)
 			break;
@@ -397,7 +386,7 @@ static int do_iterate(struct gard_ctx *ctx,
  */
 static int __gard_next(struct gard_ctx *ctx, int pos, struct gard_record *gard, int *rc)
 {
-	uint32_t offset = pos * sizeof_gard(ctx);
+	uint32_t offset = pos * sizeof(*gard);
 
 	if (offset > ctx->gard_data_len) /* too big */
 		return -1;
@@ -577,7 +566,7 @@ static int do_clear_i(struct gard_ctx *ctx, int pos, struct gard_record *gard, v
 	if (pos < largest) {
 		/* We're not clearing the last record, shift all the records up */
 		int buf_len = ((largest - pos) * sizeof(struct gard_record));
-		int buf_pos = ctx->gard_data_pos + ((pos + 1) * sizeof_gard(ctx));
+		int buf_pos = ctx->gard_data_pos + ((pos + 1) * sizeof(struct gard_record));
 		buf = malloc(buf_len);
 		if (!buf)
 			return -ENOMEM;
@@ -589,17 +578,17 @@ static int do_clear_i(struct gard_ctx *ctx, int pos, struct gard_record *gard, v
 			return rc;
 		}
 
-		rc = blocklevel_smart_write(ctx->bl, buf_pos - sizeof_gard(ctx), buf, buf_len);
+		rc = blocklevel_smart_write(ctx->bl, buf_pos - sizeof(gard), buf, buf_len);
 		free(buf);
 		if (rc) {
 			fprintf(stderr, "Couldn't write to flash at 0x%08x for len 0x%08x\n",
-			        buf_pos - (int) sizeof_gard(ctx), buf_len);
+					buf_pos - (int) sizeof(struct gard_record), buf_len);
 			return rc;
 		}
 	}
 
 	/* Now wipe the last record */
-	rc = blocklevel_smart_write(ctx->bl, ctx->gard_data_pos + (largest * sizeof_gard(ctx)),
+	rc = blocklevel_smart_write(ctx->bl, ctx->gard_data_pos + (largest * sizeof(null_gard)),
 	                            &null_gard, sizeof(null_gard));
 	printf("done\n");
 
@@ -608,27 +597,24 @@ static int do_clear_i(struct gard_ctx *ctx, int pos, struct gard_record *gard, v
 
 static int reset_partition(struct gard_ctx *ctx)
 {
-	int len, num_entries, rc = 0;
 	struct gard_record *gard;
+	int rc = 0;
 
-	num_entries = ctx->gard_data_len / sizeof_gard(ctx);
-	len = num_entries * sizeof(*gard);
-	gard = malloc(len);
+	gard = malloc(ctx->gard_data_len);
 	if (!gard) {
 		return FLASH_ERR_MALLOC_FAILED;
 	}
-	memset(gard, 0xFF, len);
+	memset(gard, 0xFF, ctx->gard_data_len);
 
 	rc = blocklevel_smart_erase(ctx->bl, ctx->gard_data_pos, ctx->gard_data_len);
 	if (rc) {
 		fprintf(stderr, "Couldn't erase the gard partition. Bailing out\n");
 		goto out;
 	}
-	rc = blocklevel_write(ctx->bl, ctx->gard_data_pos, gard, len);
-	if (rc) {
+
+	rc = blocklevel_write(ctx->bl, ctx->gard_data_pos, gard, ctx->gard_data_len);
+	if (rc)
 		fprintf(stderr, "Couldn't reset the entire gard partition. Bailing out\n");
-		goto out;
-	}
 
 out:
 	free(gard);
@@ -699,7 +685,7 @@ static int do_create(struct gard_ctx *ctx, int argc, char **argv)
 
 	/* do we have an empty record to write into? */
 	if (!rc && !is_valid_record(&gard)) {
-		int offset = last_pos * sizeof_gard(ctx);
+		int offset = last_pos * sizeof(gard);
 
 		memset(&gard, 0xff, sizeof(gard));
 
@@ -976,7 +962,7 @@ int main(int argc, char **argv)
 			goto out;
 
 		rc = ffs_part_info(ctx->ffs, ctx->gard_part_idx, NULL, &(ctx->gard_data_pos),
-				&(ctx->gard_data_len), NULL, &(ctx->ecc));
+				&(ctx->gard_data_len), NULL, NULL);
 		if (rc)
 			goto out;
 	} else {
@@ -986,7 +972,6 @@ int main(int argc, char **argv)
 				goto out;
 		}
 
-		ctx->ecc = ecc;
 		ctx->gard_data_pos = 0;
 		ctx->gard_data_len = ctx->f_size;
 	}

libflash/blocklevel.c

@@ -68,6 +68,11 @@ static int ecc_protected(struct blocklevel_device *bl, uint64_t pos, uint64_t le
 	return 0;
 }
 
+static uint64_t with_ecc_pos(uint64_t ecc_start, uint64_t pos)
+{
+	return pos + ((pos - ecc_start) / (BYTES_PER_ECC));
+}
+
 static int reacquire(struct blocklevel_device *bl)
 {
 	if (!bl->keep_alive && bl->reacquire)
@@ -112,7 +117,7 @@ int blocklevel_read(struct blocklevel_device *bl, uint64_t pos, void *buf, uint6
 {
 	int rc, ecc_protection;
 	struct ecc64 *buffer;
-	uint64_t ecc_start, ecc_len = ecc_buffer_size(len);
+	uint64_t ecc_pos, ecc_start, ecc_diff, ecc_len;
 
 	FL_DBG("%s: 0x%" PRIx64 "\t%p\t0x%" PRIx64 "\n", __func__, pos, buf, len);
 	if (!bl || !buf) {
@@ -139,18 +144,31 @@ int blocklevel_read(struct blocklevel_device *bl, uint64_t pos, void *buf, uint6
 		return FLASH_ERR_PARM_ERROR;
 	}
 
+	pos = with_ecc_pos(ecc_start, pos);
+
+	ecc_pos = ecc_buffer_align(ecc_start, pos);
+	ecc_diff = pos - ecc_pos;
+	ecc_len = ecc_buffer_size(len + ecc_diff);
+
+	FL_DBG("%s: adjusted_pos: 0x%" PRIx64 ", ecc_pos: 0x%" PRIx64
+			", ecc_diff: 0x%" PRIx64 ", ecc_len: 0x%" PRIx64 "\n",
+			__func__, pos, ecc_pos, ecc_diff, ecc_len);
 	buffer = malloc(ecc_len);
 	if (!buffer) {
 		errno = ENOMEM;
 		rc = FLASH_ERR_MALLOC_FAILED;
 		goto out;
 	}
 
-	rc = blocklevel_raw_read(bl, pos, buffer, ecc_len);
+	rc = blocklevel_raw_read(bl, ecc_pos, buffer, ecc_len);
 	if (rc)
 		goto out;
 
-	if (memcpy_from_ecc(buf, buffer, len)) {
+	/*
+	 * Could optimise and simply call memcpy_from_ecc() if ecc_diff
+	 * == 0 but _unaligned checks and bascially does that for us
+	 */
+	if (memcpy_from_ecc_unaligned(buf, buffer, len, ecc_diff)) {
 		errno = EBADF;
 		rc = FLASH_ERR_ECC_INVALID;
 	}
@@ -188,7 +206,7 @@ int blocklevel_write(struct blocklevel_device *bl, uint64_t pos, const void *buf
 	int rc, ecc_protection;
 	struct ecc64 *buffer;
 	uint64_t ecc_len = ecc_buffer_size(len);
-	uint64_t ecc_start;
+	uint64_t ecc_start, ecc_pos, ecc_diff;
 
 	FL_DBG("%s: 0x%" PRIx64 "\t%p\t0x%" PRIx64 "\n", __func__, pos, buf, len);
 	if (!bl || !buf) {
@@ -215,19 +233,61 @@ int blocklevel_write(struct blocklevel_device *bl, uint64_t pos, const void *buf
 		return FLASH_ERR_PARM_ERROR;
 	}
 
+	pos = with_ecc_pos(ecc_start, pos);
+
+	ecc_pos = ecc_buffer_align(ecc_start, pos);
+	ecc_diff = pos - ecc_pos;
+	ecc_len = ecc_buffer_size(len + ecc_diff);
+
+	FL_DBG("%s: adjusted_pos: 0x%" PRIx64 ", ecc_pos: 0x%" PRIx64
+			", ecc_diff: 0x%" PRIx64 ", ecc_len: 0x%" PRIx64 "\n",
+			__func__, pos, ecc_pos, ecc_diff, ecc_len);
+
 	buffer = malloc(ecc_len);
 	if (!buffer) {
 		errno = ENOMEM;
 		rc = FLASH_ERR_MALLOC_FAILED;
 		goto out;
 	}
 
-	if (memcpy_to_ecc(buffer, buf, len)) {
-		errno = EBADF;
-		rc = FLASH_ERR_ECC_INVALID;
-		goto out;
-	}
+	if (ecc_diff) {
+		uint64_t start_chunk = ecc_diff;
+		uint64_t end_chunk = BYTES_PER_ECC - ecc_diff;
+		uint64_t end_len = ecc_len - end_chunk;
+
+		/*
+		 * Read the start bytes that memcpy_to_ecc_unaligned() will need
+		 * to calculate the first ecc byte
+		 */
+		rc = blocklevel_raw_read(bl, ecc_pos, buffer, start_chunk);
+		if (rc) {
+			errno = EBADF;
+			rc = FLASH_ERR_ECC_INVALID;
+		}
+
+		/*
+		 * Read the end bytes that memcpy_to_ecc_unaligned() will need
+		 * to calculate the last ecc byte
+		 */
+		rc = blocklevel_raw_read(bl, ecc_pos + end_len, ((char *)buffer) + end_len,
+				end_chunk);
+		if (rc) {
+			errno = EBADF;
+			rc = FLASH_ERR_ECC_INVALID;
+		}
 
+		if (memcpy_to_ecc_unaligned(buffer, buf, len, ecc_diff)) {
+			errno = EBADF;
+			rc = FLASH_ERR_ECC_INVALID;
+			goto out;
+		}
+	} else {
+		if (memcpy_to_ecc(buffer, buf, len)) {
+			errno = EBADF;
+			rc = FLASH_ERR_ECC_INVALID;
+			goto out;
+		}
+	}
 	rc = blocklevel_raw_write(bl, pos, buffer, ecc_len);
 
 out: