Skip to contentSkip to conversation

efi-pstore: Fix read iter after pstore API refactor

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

Subsystems (2)
lore
Share & tools
lore

Kees Cook

author
#0 lore raw

PATCH 1/1 efi-pstore: Fix read iter after pstore API refactor

During the internal pstore API refactoring, the EFI vars read entry was accidentally made to update a stack variable instead of the pstore private data pointer. This corrects the problem (and removes the now needless argument).

---
 drivers/firmware/efi/efi-pstore.c | 26 ++++++++++----------------
 1 file changed, 10 insertions(+), 16 deletions(-)
drivers/firmware/efi/efi-pstore.c +10 -16
index 93d8cdbe7ef4..9e6f14c354f1 100644
@@ -155,25 +155,20 @@ static int efi_pstore_scan_sysfs_exit(struct efivar_entry *pos,
  * efi_pstore_sysfs_entry_iter
  *
  * @record: pstore record to pass to callback
- * @pos: entry to begin iterating from
  *
  * You MUST call efivar_enter_iter_begin() before this function, and
  * efivar_entry_iter_end() afterwards.
  *
- * It is possible to begin iteration from an arbitrary entry within
- * the list by passing @pos. @pos is updated on return to point to
- * the next entry of the last one passed to efi_pstore_read_func().
- * To begin iterating from the beginning of the list @pos must be %NULL.
  */
-static int efi_pstore_sysfs_entry_iter(struct pstore_record *record,
-				       struct efivar_entry **pos)
+static int efi_pstore_sysfs_entry_iter(struct pstore_record *record)
 {
+	struct efivar_entry *pos = (struct efivar_entry *)record->psi->data;
 	struct efivar_entry *entry, *n;
 	struct list_head *head = &efivar_sysfs_list;
 	int size = 0;
 	int ret;
 
-	if (!*pos) {
+	if (!pos) {
 		list_for_each_entry_safe(entry, n, head, list) {
 			efi_pstore_scan_sysfs_enter(entry, n, head);
 
@@ -185,21 +180,21 @@ static int efi_pstore_sysfs_entry_iter(struct pstore_record *record,
 			if (size)
 				break;
 		}
-		*pos = n;
+		pos = n;
 		return size;
 	}
 
-	list_for_each_entry_safe_from((*pos), n, head, list) {
-		efi_pstore_scan_sysfs_enter((*pos), n, head);
+	list_for_each_entry_safe_from(pos, n, head, list) {
+		efi_pstore_scan_sysfs_enter(pos, n, head);
 
-		size = efi_pstore_read_func((*pos), record);
-		ret = efi_pstore_scan_sysfs_exit((*pos), n, head, size < 0);
+		size = efi_pstore_read_func(pos, record);
+		ret = efi_pstore_scan_sysfs_exit(pos, n, head, size < 0);
 		if (ret)
 			return ret;
 		if (size)
 			break;
 	}
-	*pos = n;
+	pos = n;
 	return size;
 }
 
@@ -218,7 +213,6 @@ static int efi_pstore_sysfs_entry_iter(struct pstore_record *record,
  */
 static ssize_t efi_pstore_read(struct pstore_record *record)
 {
-	struct efivar_entry *entry = (struct efivar_entry *)record->psi->data;
 	ssize_t size;
 
 	record->buf = kzalloc(EFIVARS_DATA_SIZE_MAX, GFP_KERNEL);
@@ -229,7 +223,7 @@ static ssize_t efi_pstore_read(struct pstore_record *record)
 		size = -EINTR;
 		goto out;
 	}
-	size = efi_pstore_sysfs_entry_iter(record, &entry);
+	size = efi_pstore_sysfs_entry_iter(record);
 	efivar_entry_iter_end();
 
 out:
Signature · 6 lines
2.7.4


-- 
Kees Cook
Pixel Security
#1 lore raw
Quoted 6 lines · Kees Cook

During the internal pstore API refactoring, the EFI vars read entry was accidentally made to update a stack variable instead of the pstore private data pointer. This corrects the problem (and removes the now needless argument).

Does this need a cc stable?

Quoted 89 lines
---
 drivers/firmware/efi/efi-pstore.c | 26 ++++++++++----------------
 1 file changed, 10 insertions(+), 16 deletions(-)
drivers/firmware/efi/efi-pstore.c +10 -17
index 93d8cdbe7ef4..9e6f14c354f1 100644
@@ -155,25 +155,20 @@ static int efi_pstore_scan_sysfs_exit(struct efivar_entry *pos,
  * efi_pstore_sysfs_entry_iter
  *
  * @record: pstore record to pass to callback
- * @pos: entry to begin iterating from
  *
  * You MUST call efivar_enter_iter_begin() before this function, and
  * efivar_entry_iter_end() afterwards.
  *
- * It is possible to begin iteration from an arbitrary entry within
- * the list by passing @pos. @pos is updated on return to point to
- * the next entry of the last one passed to efi_pstore_read_func().
- * To begin iterating from the beginning of the list @pos must be %NULL.
  */
-static int efi_pstore_sysfs_entry_iter(struct pstore_record *record,
-                                      struct efivar_entry **pos)
+static int efi_pstore_sysfs_entry_iter(struct pstore_record *record)
 {
+       struct efivar_entry *pos = (struct efivar_entry *)record->psi->data;
        struct efivar_entry *entry, *n;
        struct list_head *head = &efivar_sysfs_list;
        int size = 0;
        int ret;
 
-       if (!*pos) {
+       if (!pos) {
                list_for_each_entry_safe(entry, n, head, list) {
                        efi_pstore_scan_sysfs_enter(entry, n, head);
 
@@ -185,21 +180,21 @@ static int efi_pstore_sysfs_entry_iter(struct pstore_record *record,
                        if (size)
                                break;
                }
-               *pos = n;
+               pos = n;
                return size;
        }
 
-       list_for_each_entry_safe_from((*pos), n, head, list) {
-               efi_pstore_scan_sysfs_enter((*pos), n, head);
+       list_for_each_entry_safe_from(pos, n, head, list) {
+               efi_pstore_scan_sysfs_enter(pos, n, head);
 
-               size = efi_pstore_read_func((*pos), record);
-               ret = efi_pstore_scan_sysfs_exit((*pos), n, head, size < 0);
+               size = efi_pstore_read_func(pos, record);
+               ret = efi_pstore_scan_sysfs_exit(pos, n, head, size < 0);
                if (ret)
                        return ret;
                if (size)
                        break;
        }
-       *pos = n;
+       pos = n;
        return size;
 }
 
@@ -218,7 +213,6 @@ static int efi_pstore_sysfs_entry_iter(struct pstore_record *record,
  */
 static ssize_t efi_pstore_read(struct pstore_record *record)
 {
-       struct efivar_entry *entry = (struct efivar_entry *)record->psi->data;
        ssize_t size;
 
        record->buf = kzalloc(EFIVARS_DATA_SIZE_MAX, GFP_KERNEL);
@@ -229,7 +223,7 @@ static ssize_t efi_pstore_read(struct pstore_record *record)
                size = -EINTR;
                goto out;
        }
-       size = efi_pstore_sysfs_entry_iter(record, &entry);
+       size = efi_pstore_sysfs_entry_iter(record);
        efivar_entry_iter_end();
 
 out:
--

2.7.4

--
Kees Cook
Pixel Security

author
#2 lore raw
Quoted 9 lines · Ard Biesheuvel
Quoted 6 lines · Kees Cook

During the internal pstore API refactoring, the EFI vars read entry was accidentally made to update a stack variable instead of the pstore private data pointer. This corrects the problem (and removes the now needless argument).

Does this need a cc stable?

No, the refactor first appeared in 4.12-rc1.

-Kees

Signature · 2 lines
Kees Cook
Pixel Security
#3 lore raw
Quoted 8 lines · Kees Cook
Quoted 6 lines · Ard Biesheuvel
Quoted 5 lines · Kees Cook

During the internal pstore API refactoring, the EFI vars read entry was accidentally made to update a stack variable instead of the pstore private data pointer. This corrects the problem (and removes the now needless argument).

Quoted 6 lines
Quoted 3 lines
Quoted 1 line

Does this need a cc stable?

No, the refactor first appeared in 4.12-rc1.

OK. Applied.

author
#4 lore raw
Quoted 9 lines · Ard Biesheuvel
Quoted 8 lines · Kees Cook
Quoted 6 lines · Ard Biesheuvel
Quoted 5 lines · Kees Cook

During the internal pstore API refactoring, the EFI vars read entry was accidentally made to update a stack variable instead of the pstore private data pointer. This corrects the problem (and removes the now needless argument).

Quoted 8 lines
Quoted 6 lines
Quoted 3 lines
Quoted 1 line

Does this need a cc stable?

No, the refactor first appeared in 4.12-rc1.

OK. Applied.

Err, eek, no, it's already gone into Linus's tree.

-Kees

Signature · 2 lines
Kees Cook
Pixel Security
#5 lore raw
Quoted 13 lines · Kees Cook
Quoted 11 lines · Ard Biesheuvel
Quoted 10 lines · Kees Cook
Quoted 8 lines · Ard Biesheuvel
Quoted 6 lines · Kees Cook

During the internal pstore API refactoring, the EFI vars read entry was accidentally made to update a stack variable instead of the pstore private data pointer. This corrects the problem (and removes the now needless argument).

Quoted 9 lines
Quoted 6 lines
Quoted 4 lines
Quoted 1 line

Does this need a cc stable?

No, the refactor first appeared in 4.12-rc1.

OK. Applied.

Err, eek, no, it's already gone into Linus's tree.

OK, I dropped it again :-)