All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] efi-pstore: Fix write/erase id tracking
@ 2017-05-19 18:27 ` Kees Cook
  0 siblings, 0 replies; 4+ messages in thread
From: Kees Cook @ 2017-05-19 18:27 UTC (permalink / raw)
  To: Marta Lofstedt
  Cc: Anton Vorontsov, Colin Cross, Tony Luck, Matt Fleming,
	Ard Biesheuvel, linux-efi, linux-kernel

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 <marta.lofstedt@intel.com>
Fixes: 76cc9580e3fb ("pstore: Replace arguments for write() API")
Fixes: a61072aae693 ("pstore: Replace arguments for erase() API")
Signed-off-by: Kees Cook <keescook@chromium.org>
---
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(-)

diff --git a/drivers/firmware/efi/efi-pstore.c b/drivers/firmware/efi/efi-pstore.c
index 44148fd4c9f2..dda2e96328c0 100644
--- a/drivers/firmware/efi/efi-pstore.c
+++ b/drivers/firmware/efi/efi-pstore.c
@@ -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);
-- 
2.7.4


-- 
Kees Cook
Pixel Security

^ permalink raw reply related	[flat|nested] 4+ messages in thread
* [PATCH] efi-pstore: Fix write/erase id tracking
@ 2017-05-19 18:27 ` Kees Cook
  0 siblings, 0 replies; 4+ messages in thread
From: Kees Cook @ 2017-05-19 18:27 UTC (permalink / raw)
  To: Marta Lofstedt
  Cc: Anton Vorontsov, Colin Cross, Tony Luck, Matt Fleming,
	Ard Biesheuvel, linux-efi-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA

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 <marta.lofstedt-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Fixes: 76cc9580e3fb ("pstore: Replace arguments for write() API")
Fixes: a61072aae693 ("pstore: Replace arguments for erase() API")
Signed-off-by: Kees Cook <keescook-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
---
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(-)

diff --git a/drivers/firmware/efi/efi-pstore.c b/drivers/firmware/efi/efi-pstore.c
index 44148fd4c9f2..dda2e96328c0 100644
--- a/drivers/firmware/efi/efi-pstore.c
+++ b/drivers/firmware/efi/efi-pstore.c
@@ -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);
-- 
2.7.4


-- 
Kees Cook
Pixel Security

^ permalink raw reply related	[flat|nested] 4+ messages in thread
* RE: [PATCH] efi-pstore: Fix write/erase id tracking
  2017-05-19 18:27 ` Kees Cook
@ 2017-05-22  6:51   ` Lofstedt, Marta
  -1 siblings, 0 replies; 4+ messages in thread
From: Lofstedt, Marta @ 2017-05-22  6:51 UTC (permalink / raw)
  To: Kees Cook
  Cc: Anton Vorontsov, Colin Cross, Luck, Tony, Matt Fleming,
	Ard Biesheuvel, linux-efi, linux-kernel

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

> -----Original Message-----
> From: Kees Cook [mailto:keescook@chromium.org]
> Sent: Friday, May 19, 2017 9:27 PM
> To: Lofstedt, Marta <marta.lofstedt@intel.com>
> Cc: Anton Vorontsov <anton@enomsg.org>; Colin Cross
> <ccross@android.com>; Luck, Tony <tony.luck@intel.com>; Matt Fleming
> <matt@codeblueprint.co.uk>; Ard Biesheuvel <ard.biesheuvel@linaro.org>;
> linux-efi@vger.kernel.org; linux-kernel@vger.kernel.org
> 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 <marta.lofstedt@intel.com>
> Fixes: 76cc9580e3fb ("pstore: Replace arguments for write() API")
> Fixes: a61072aae693 ("pstore: Replace arguments for erase() API")
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
> 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(-)
> 
> diff --git a/drivers/firmware/efi/efi-pstore.c b/drivers/firmware/efi/efi-
> pstore.c
> index 44148fd4c9f2..dda2e96328c0 100644
> --- a/drivers/firmware/efi/efi-pstore.c
> +++ b/drivers/firmware/efi/efi-pstore.c
> @@ -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);
> --
> 2.7.4
> 
> 
> --
> Kees Cook
> Pixel Security

^ permalink raw reply	[flat|nested] 4+ messages in thread
* RE: [PATCH] efi-pstore: Fix write/erase id tracking
@ 2017-05-22  6:51   ` Lofstedt, Marta
  0 siblings, 0 replies; 4+ messages in thread
From: Lofstedt, Marta @ 2017-05-22  6:51 UTC (permalink / raw)
  To: Kees Cook
  Cc: Anton Vorontsov, Colin Cross, Luck, Tony, Matt Fleming,
	Ard Biesheuvel, linux-efi-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA

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

> -----Original Message-----
> From: Kees Cook [mailto:keescook-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org]
> Sent: Friday, May 19, 2017 9:27 PM
> To: Lofstedt, Marta <marta.lofstedt-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> Cc: Anton Vorontsov <anton-9xeibp6oKSgdnm+yROfE0A@public.gmane.org>; Colin Cross
> <ccross-z5hGa2qSFaRBDgjK7y7TUQ@public.gmane.org>; Luck, Tony <tony.luck-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>; Matt Fleming
> <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org>; Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>;
> linux-efi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> 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 <marta.lofstedt-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> Fixes: 76cc9580e3fb ("pstore: Replace arguments for write() API")
> Fixes: a61072aae693 ("pstore: Replace arguments for erase() API")
> Signed-off-by: Kees Cook <keescook-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> ---
> 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(-)
> 
> diff --git a/drivers/firmware/efi/efi-pstore.c b/drivers/firmware/efi/efi-
> pstore.c
> index 44148fd4c9f2..dda2e96328c0 100644
> --- a/drivers/firmware/efi/efi-pstore.c
> +++ b/drivers/firmware/efi/efi-pstore.c
> @@ -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);
> --
> 2.7.4
> 
> 
> --
> Kees Cook
> Pixel Security

^ permalink raw reply	[flat|nested] 4+ messages in thread
end of thread, other threads:[~2017-05-22  6:51 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-05-19 18:27 [PATCH] efi-pstore: Fix write/erase id tracking Kees Cook
2017-05-19 18:27 ` Kees Cook
2017-05-22  6:51 ` Lofstedt, Marta
2017-05-22  6:51   ` Lofstedt, Marta

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.