efi-pstore: Fix write/erase id tracking

PATCH In review Kees Cook 2 participants 2 messages May 19, 2017 – May 22, 2017

Subsystems (2)

EFI Pstore filesystem

lore

Share & tools

lore

Kees Cook

author

2017-05-19 18:27 UTC #0 lore raw

PATCH 1/1 efi-pstore: Fix write/erase id tracking

Prior to the pstore interface refactoring, the "id" generated during a backend pstore_write() was only retained by the internal pstore inode tracking list. Additionally the "part" was ignored, so EFI would encode this in the id. This corrects the misunderstandings and correctly sets "id" during pstore_write(), and uses "part" directly during pstore_erase().

Reported-by Marta Lofstedt <[email protected]>

Fixes 76cc9580e3fb ("pstore: Replace arguments for write() API")

Fixes a61072aae693 ("pstore: Replace arguments for erase() API")

Signed-off-by Kees Cook <[email protected]>

---
Since the pstore refactor broke this, I intend to push this via the pstore tree.

---
 drivers/firmware/efi/efi-pstore.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

drivers/firmware/efi/efi-pstore.c +11 -6

index 44148fd4c9f2..dda2e96328c0 100644
@@ -53,6 +53,7 @@ static int efi_pstore_read_func(struct efivar_entry *entry,
 	if (sscanf(name, "dump-type%u-%u-%d-%lu-%c",
 		   &record->type, &part, &cnt, &time, &data_type) == 5) {
 		record->id = generic_id(time, part, cnt);
+		record->part = part;
 		record->count = cnt;
 		record->time.tv_sec = time;
 		record->time.tv_nsec = 0;

@@ -64,6 +65,7 @@ static int efi_pstore_read_func(struct efivar_entry *entry,
 	} else if (sscanf(name, "dump-type%u-%u-%d-%lu",
 		   &record->type, &part, &cnt, &time) == 4) {
 		record->id = generic_id(time, part, cnt);
+		record->part = part;
 		record->count = cnt;
 		record->time.tv_sec = time;
 		record->time.tv_nsec = 0;

@@ -77,6 +79,7 @@ static int efi_pstore_read_func(struct efivar_entry *entry,
 		 * multiple logs, remains.
 		 */
 		record->id = generic_id(time, part, 0);
+		record->part = part;
 		record->count = 0;
 		record->time.tv_sec = time;
 		record->time.tv_nsec = 0;

@@ -241,9 +244,15 @@ static int efi_pstore_write(struct pstore_record *record)
 	efi_guid_t vendor = LINUX_EFI_CRASH_GUID;
 	int i, ret = 0;
 
+	record->time.tv_sec = get_seconds();
+	record->time.tv_nsec = 0;
+
+	record->id = generic_id(record->time.tv_sec, record->part,
+				record->count);
+
 	snprintf(name, sizeof(name), "dump-type%u-%u-%d-%lu-%c",
 		 record->type, record->part, record->count,
-		 get_seconds(), record->compressed ? 'C' : 'D');
+		 record->time.tv_sec, record->compressed ? 'C' : 'D');
 
 	for (i = 0; i < DUMP_NAME_LEN; i++)
 		efi_name[i] = name[i];

@@ -255,7 +264,6 @@ static int efi_pstore_write(struct pstore_record *record)
 	if (record->reason == KMSG_DUMP_OOPS)
 		efivar_run_worker();
 
-	record->id = record->part;
 	return ret;
 };

@@ -287,7 +295,7 @@ static int efi_pstore_erase_func(struct efivar_entry *entry, void *data)
 		 * holding multiple logs, remains.
 		 */
 		snprintf(name_old, sizeof(name_old), "dump-type%u-%u-%lu",
-			ed->record->type, (unsigned int)ed->record->id,
+			ed->record->type, ed->record->part,
 			ed->record->time.tv_sec);
 
 		for (i = 0; i < DUMP_NAME_LEN; i++)

@@ -320,10 +328,7 @@ static int efi_pstore_erase(struct pstore_record *record)
 	char name[DUMP_NAME_LEN];
 	efi_char16_t efi_name[DUMP_NAME_LEN];
 	int found, i;
-	unsigned int part;
 
-	do_div(record->id, 1000);
-	part = do_div(record->id, 100);
 	snprintf(name, sizeof(name), "dump-type%u-%u-%d-%lu",
 		 record->type, record->part, record->count,
 		 record->time.tv_sec);

Signature · 6 lines

2.7.4


-- 
Kees Cook
Pixel Security

Lofstedt, Marta

2017-05-22 06:51 UTC #1 lore raw

Thanks Kees,

I confirm that with below patch on top of 4.12.0-rc2 pstore efi vars is now working as expected again.

BR,
Marta

Quoted 121 lines

-----Original Message-----
From: Kees Cook [mailto:[email protected]]
Sent: Friday, May 19, 2017 9:27 PM
To: Lofstedt, Marta <[email protected]>
Cc: Anton Vorontsov <[email protected]>; Colin Cross <[email protected]>; Luck, Tony <[email protected]>; Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/[email protected]>; Ard Biesheuvel <[email protected]>; [email protected]; [email protected]
Subject: [PATCH] efi-pstore: Fix write/erase id tracking
Prior to the pstore interface refactoring, the "id" generated during a backend pstore_write() was only retained by the internal pstore inode tracking list. Additionally the "part" was ignored, so EFI would encode this in the id. This corrects the misunderstandings and correctly sets "id" during pstore_write(), and uses "part"
directly during pstore_erase().
Reported-by Marta Lofstedt <[email protected]>
Fixes 76cc9580e3fb ("pstore: Replace arguments for write() API")
Fixes a61072aae693 ("pstore: Replace arguments for erase() API")
Signed-off-by Kees Cook <[email protected]>
---
Since the pstore refactor broke this, I intend to push this via the pstore tree.
---
 drivers/firmware/efi/efi-pstore.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)
drivers/firmware/efi/efi- renamed from drivers/firmware/efi/efi-pstore.c +0 -0
pstore.c
index 44148fd4c9f2..dda2e96328c0 100644
drivers/firmware/efi/efi-pstore.c +1 -0
@@ -53,6 +53,7 @@ static int efi_pstore_read_func(struct efivar_entry
 *entry,
 	if (sscanf(name, "dump-type%u-%u-%d-%lu-%c",
 		   &record->type, &part, &cnt, &time,
 &data_type) == 5) {
 		record->id = generic_id(time, part, cnt);
+		record->part = part;
 		record->count = cnt;
drivers/firmware/efi/efi-pstore.c +1 -0
 		record->time.tv_sec = time;
 		record->time.tv_nsec = 0;
@@ -64,6 +65,7 @@ static int efi_pstore_read_func(struct efivar_entry
 *entry,
 	} else if (sscanf(name, "dump-type%u-%u-%d-%lu",
 		   &record->type, &part, &cnt, &time) == 4) {
 		record->id = generic_id(time, part, cnt);
+		record->part = part;
 		record->count = cnt;
 		record->time.tv_sec = time;
drivers/firmware/efi/efi-pstore.c +1 -0
 		record->time.tv_nsec = 0;
@@ -77,6 +79,7 @@ static int efi_pstore_read_func(struct efivar_entry
 *entry,
 		 * multiple logs, remains.
 		 */
 		record->id = generic_id(time, part, 0);
+		record->part = part;
 		record->count = 0;
 		record->time.tv_sec = time;
drivers/firmware/efi/efi-pstore.c +7 -1
 		record->time.tv_nsec = 0;
@@ -241,9 +244,15 @@ static int efi_pstore_write(struct pstore_record
 *record)
 	efi_guid_t vendor = LINUX_EFI_CRASH_GUID;
 	int i, ret = 0;
 
+	record->time.tv_sec = get_seconds();
+	record->time.tv_nsec = 0;
+
+	record->id = generic_id(record->time.tv_sec, record->part,
+				record->count);
+
 	snprintf(name, sizeof(name), "dump-type%u-%u-%d-%lu-
 %c",
 		 record->type, record->part, record->count,
-		 get_seconds(), record->compressed ? 'C' : 'D');
+		 record->time.tv_sec, record->compressed ? 'C'
 : 'D');
drivers/firmware/efi/efi-pstore.c +1 -2
 	for (i = 0; i < DUMP_NAME_LEN; i++)
 		efi_name[i] = name[i];
@@ -255,7 +264,6 @@ static int efi_pstore_write(struct pstore_record
 *record)
 	if (record->reason == KMSG_DUMP_OOPS)
 		efivar_run_worker();
 
-	record->id = record->part;
 	return ret;
 };
@@ -287,7 +295,7 @@ static int efi_pstore_erase_func(struct efivar_entry
 *entry, void *data)
 		 * holding multiple logs, remains.
 		 */
 		snprintf(name_old, sizeof(name_old), "dump-
 type%u-%u-%lu",
-			ed->record->type, (unsigned
 int)ed->record->id,
+			ed->record->type, ed->record-
Quoted 1 line
part,
drivers/firmware/efi/efi-pstore.c +0 -3
 			ed->record->time.tv_sec);
 
 		for (i = 0; i < DUMP_NAME_LEN; i++)
@@ -320,10 +328,7 @@ static int efi_pstore_erase(struct pstore_record
 *record)
 	char name[DUMP_NAME_LEN];
 	efi_char16_t efi_name[DUMP_NAME_LEN];
 	int found, i;
-	unsigned int part;
 
-	do_div(record->id, 1000);
-	part = do_div(record->id, 100);
 	snprintf(name, sizeof(name), "dump-type%u-%u-%d-%lu",
 		 record->type, record->part, record->count,
record->time.tv_sec);
--
2.7.4
--
Kees Cook
Pixel Security