Re: [PATCH 06/18] pstore: Extract common arguments into structure

[Namhyung Kim]

On Tue, Mar 7, 2017 at 6:55 AM, Kees Cook  wrote:
> The read/mkfile pair pass the same arguments and should be cleared
> between calls. Move to a structure and wipe it after every loop.
>
> Signed-off-by: Kees Cook 
> ---
>  fs/pstore/platform.c   | 55 
> +++---
>  include/linux/pstore.h | 28 -
>  2 files changed, 57 insertions(+), 26 deletions(-)
>
> diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
> index 320a673ecb5b..3fa1575a6e36 100644
> --- a/fs/pstore/platform.c
> +++ b/fs/pstore/platform.c
> @@ -766,16 +766,9 @@ EXPORT_SYMBOL_GPL(pstore_unregister);
>  void pstore_get_records(int quiet)
>  {
> struct pstore_info *psi = psinfo;
> -   char*buf = NULL;
> -   ssize_t size;
> -   u64 id;
> -   int count;
> -   enum pstore_type_id type;
> -   struct timespec time;
> +   struct pstore_recordrecord = { .psi = psi, };
> int failed = 0, rc;
> -   boolcompressed;
> int unzipped_len = -1;
> -   ssize_t ecc_notice_size = 0;
>
> if (!psi)
> return;
> @@ -784,39 +777,51 @@ void pstore_get_records(int quiet)
> if (psi->open && psi->open(psi))
> goto out;
>
> -   while ((size = psi->read(, , , , , ,
> -_notice_size, psi)) > 0) {
> -   if (compressed && (type == PSTORE_TYPE_DMESG)) {
> +   while ((record.size = psi->read(, ,
> +, ,
> +, ,
> +_notice_size,
> +record.psi)) > 0) {
> +   if (record.compressed &&
> +   record.type == PSTORE_TYPE_DMESG) {
> if (big_oops_buf)
> -   unzipped_len = pstore_decompress(buf,
> -   big_oops_buf, size,
> +   unzipped_len = pstore_decompress(
> +   record.buf,
> +   big_oops_buf,
> +   record.size,
> big_oops_buf_sz);
>
> if (unzipped_len > 0) {
> -   if (ecc_notice_size)
> +   if (record.ecc_notice_size)
> memcpy(big_oops_buf + unzipped_len,
> -  buf + size, ecc_notice_size);
> -   kfree(buf);
> -   buf = big_oops_buf;
> -   size = unzipped_len;
> -   compressed = false;
> +  record.buf + recorrecord.size,

A typo on record.size.

Thanks,
Namhyung

Re: [PATCH 06/18] pstore: Extract common arguments into structure

[Kees Cook]

On Tue, Mar 7, 2017 at 8:22 AM, Namhyung Kim  wrote:
> On Tue, Mar 7, 2017 at 6:55 AM, Kees Cook  wrote:
>> The read/mkfile pair pass the same arguments and should be cleared
>> between calls. Move to a structure and wipe it after every loop.
>>
>> Signed-off-by: Kees Cook 
>> ---
>>  fs/pstore/platform.c   | 55 
>> +++---
>>  include/linux/pstore.h | 28 -
>>  2 files changed, 57 insertions(+), 26 deletions(-)
>>
>> diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
>> index 320a673ecb5b..3fa1575a6e36 100644
>> --- a/fs/pstore/platform.c
>> +++ b/fs/pstore/platform.c
>> @@ -766,16 +766,9 @@ EXPORT_SYMBOL_GPL(pstore_unregister);
>>  void pstore_get_records(int quiet)
>>  {
>> struct pstore_info *psi = psinfo;
>> -   char*buf = NULL;
>> -   ssize_t size;
>> -   u64 id;
>> -   int count;
>> -   enum pstore_type_id type;
>> -   struct timespec time;
>> +   struct pstore_recordrecord = { .psi = psi, };
>> int failed = 0, rc;
>> -   boolcompressed;
>> int unzipped_len = -1;
>> -   ssize_t ecc_notice_size = 0;
>>
>> if (!psi)
>> return;
>> @@ -784,39 +777,51 @@ void pstore_get_records(int quiet)
>> if (psi->open && psi->open(psi))
>> goto out;
>>
>> -   while ((size = psi->read(, , , , , 
>> ,
>> -_notice_size, psi)) > 0) {
>> -   if (compressed && (type == PSTORE_TYPE_DMESG)) {
>> +   while ((record.size = psi->read(, ,
>> +, ,
>> +, ,
>> +_notice_size,
>> +record.psi)) > 0) {
>> +   if (record.compressed &&
>> +   record.type == PSTORE_TYPE_DMESG) {
>> if (big_oops_buf)
>> -   unzipped_len = pstore_decompress(buf,
>> -   big_oops_buf, size,
>> +   unzipped_len = pstore_decompress(
>> +   record.buf,
>> +   big_oops_buf,
>> +   record.size,
>> big_oops_buf_sz);
>>
>> if (unzipped_len > 0) {
>> -   if (ecc_notice_size)
>> +   if (record.ecc_notice_size)
>> memcpy(big_oops_buf + unzipped_len,
>> -  buf + size, ecc_notice_size);
>> -   kfree(buf);
>> -   buf = big_oops_buf;
>> -   size = unzipped_len;
>> -   compressed = false;
>> +  record.buf + recorrecord.size,
>
> A typo on record.size.

Thanks! Yeah, 0-day noticed this too. I've refreshed the patches in my
tree with the correction now.

-Kees

-- 
Kees Cook
Pixel Security

[PATCH 06/18] pstore: Extract common arguments into structure

[Kees Cook]

The read/mkfile pair pass the same arguments and should be cleared
between calls. Move to a structure and wipe it after every loop.

Signed-off-by: Kees Cook 
---
 fs/pstore/platform.c   | 55 +++---
 include/linux/pstore.h | 28 -
 2 files changed, 57 insertions(+), 26 deletions(-)

diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
index 320a673ecb5b..3fa1575a6e36 100644
--- a/fs/pstore/platform.c
+++ b/fs/pstore/platform.c
@@ -766,16 +766,9 @@ EXPORT_SYMBOL_GPL(pstore_unregister);
 void pstore_get_records(int quiet)
 {
struct pstore_info *psi = psinfo;
-   char*buf = NULL;
-   ssize_t size;
-   u64 id;
-   int count;
-   enum pstore_type_id type;
-   struct timespec time;
+   struct pstore_recordrecord = { .psi = psi, };
int failed = 0, rc;
-   boolcompressed;
int unzipped_len = -1;
-   ssize_t ecc_notice_size = 0;
 
if (!psi)
return;
@@ -784,39 +777,51 @@ void pstore_get_records(int quiet)
if (psi->open && psi->open(psi))
goto out;
 
-   while ((size = psi->read(, , , , , ,
-_notice_size, psi)) > 0) {
-   if (compressed && (type == PSTORE_TYPE_DMESG)) {
+   while ((record.size = psi->read(, ,
+, ,
+, ,
+_notice_size,
+record.psi)) > 0) {
+   if (record.compressed &&
+   record.type == PSTORE_TYPE_DMESG) {
if (big_oops_buf)
-   unzipped_len = pstore_decompress(buf,
-   big_oops_buf, size,
+   unzipped_len = pstore_decompress(
+   record.buf,
+   big_oops_buf,
+   record.size,
big_oops_buf_sz);
 
if (unzipped_len > 0) {
-   if (ecc_notice_size)
+   if (record.ecc_notice_size)
memcpy(big_oops_buf + unzipped_len,
-  buf + size, ecc_notice_size);
-   kfree(buf);
-   buf = big_oops_buf;
-   size = unzipped_len;
-   compressed = false;
+  record.buf + recorrecord.size,
+  record.ecc_notice_size);
+   kfree(record.buf);
+   record.buf = big_oops_buf;
+   record.size = unzipped_len;
+   record.compressed = false;
} else {
pr_err("decompression failed;returned %d\n",
   unzipped_len);
-   compressed = true;
+   record.compressed = true;
}
}
-   rc = pstore_mkfile(type, psi->name, id, count, buf,
-  compressed, size + ecc_notice_size,
-  time, psi);
+   rc = pstore_mkfile(record.type, psi->name, record.id,
+  record.count, record.buf,
+  record.compressed,
+  record.size + record.ecc_notice_size,
+  record.time, record.psi);
if (unzipped_len < 0) {
/* Free buffer other than big oops */
-   kfree(buf);
-   buf = NULL;
+   kfree(record.buf);
+   record.buf = NULL;
} else
unzipped_len = -1;
if (rc && (rc != -EEXIST || !quiet))
failed++;
+
+   memset(, 0, sizeof(record));
+   record.psi = psi;
}
if (psi->close)
psi->close(psi);
diff --git a/include/linux/pstore.h b/include/linux/pstore.h
index 083b10bacd4a..7b25f7f17915 100644
--- a/include/linux/pstore.h
+++ b/include/linux/pstore.h
@@ -30,6 +30,8 @@
 #include 
 #include 
 
+struct module;
+
 /* pstore record types (see fs/pstore/inode.c for filename templates) */
 enum pstore_type_id {
PSTORE_TYPE_DMESG   = 0,
@@ -45,7 +47,31 @@ enum