[PATCH 1/2] tpm1_bios_measurements_next should increase position index

[PATCH 1/2] tpm1_bios_measurements_next should increase position index

[Vasily Averin]

if seq_file .next fuction does not change position index,
read after non-zero llseek can generate unexpected output.

https://bugzilla.kernel.org/show_bug.cgi?id=206283
Signed-off-by: Vasily Averin <vvs@virtuozzo.com>
---
 drivers/char/tpm/eventlog/tpm1.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/char/tpm/eventlog/tpm1.c b/drivers/char/tpm/eventlog/tpm1.c
index 739b1d9..2c96977 100644
--- a/drivers/char/tpm/eventlog/tpm1.c
+++ b/drivers/char/tpm/eventlog/tpm1.c
@@ -115,6 +115,7 @@ static void *tpm1_bios_measurements_next(struct seq_file *m, void *v,
 	u32 converted_event_size;
 	u32 converted_event_type;
 
+	(*pos)++;
 	converted_event_size = do_endian_conversion(event->event_size);
 
 	v += sizeof(struct tcpa_event) + converted_event_size;
@@ -132,7 +133,6 @@ static void *tpm1_bios_measurements_next(struct seq_file *m, void *v,
 	    ((v + sizeof(struct tcpa_event) + converted_event_size) > limit))
 		return NULL;
 
-	(*pos)++;
 	return v;
 }
 
-- 
1.8.3.1

Re: [PATCH 1/2] tpm1_bios_measurements_next should increase position index

[Jarkko Sakkinen]

On Thu, 2020-01-23 at 10:48 +0300, Vasily Averin wrote:
> if seq_file .next fuction does not change position index,
> read after non-zero llseek can generate unexpected output.
> 
> https://bugzilla.kernel.org/show_bug.cgi?id=206283
> Signed-off-by: Vasily Averin <vvs@virtuozzo.com>

* Should be "tpm: tpm1_bios_measurements_next should increase position index"
* Sentences in English start with a capital letter.
* Should probably have Fixes tag.
* Shoud have "Cc: stable@vger.kernel.org" tag.

Same remarks for TPM2 patch.

/Jarkko

Re: [PATCH 1/2] tpm1_bios_measurements_next should increase position index

[Jarkko Sakkinen]

On Wed, 2020-01-29 at 15:23 +0200, Jarkko Sakkinen wrote:
> On Thu, 2020-01-23 at 10:48 +0300, Vasily Averin wrote:
> > if seq_file .next fuction does not change position index,
> > read after non-zero llseek can generate unexpected output.
> > 
> > https://bugzilla.kernel.org/show_bug.cgi?id=206283
> > Signed-off-by: Vasily Averin <vvs@virtuozzo.com>
> 
> * Should be "tpm: tpm1_bios_measurements_next should increase position index"
> * Sentences in English start with a capital letter.
> * Should probably have Fixes tag.
> * Shoud have "Cc: stable@vger.kernel.org" tag.
> 
> Same remarks for TPM2 patch.

* Shoud have explanation what kind of output is generated
  without the fix. "Unexpected output" is does not document
  the regression.

/Jarkko

[PATCH v2 0/2] tpm: seq_file .next functions should increase position index

[Vasily Averin]

v2: improved patch description

In Aug 2018 NeilBrown noticed 
commit 1f4aace60b0e ("fs/seq_file.c: simplify seq_file iteration code and interface")
"Some ->next functions do not increment *pos when they return NULL...
Note that such ->next functions are buggy and should be fixed. 
A simple demonstration is
   
dd if=/proc/swaps bs=1000 skip=1
    
Choose any block size larger than the size of /proc/swaps.  This will
always show the whole last line of /proc/swaps"

Described problem is still actual. If you make lseek into middle of last output line 
following read will output end of last line and whole last line once again.

$ dd if=/proc/swaps bs=1  # usual output
Filename				Type		Size	Used	Priority
/dev/dm-0                               partition	4194812	97536	-2
104+0 records in
104+0 records out
104 bytes copied

$ dd if=/proc/swaps bs=40 skip=1    # last line was generated twice
dd: /proc/swaps: cannot skip to specified offset
v/dm-0                               partition	4194812	97536	-2
/dev/dm-0                               partition	4194812	97536	-2 
3+1 records in
3+1 records out
131 bytes copied

There are lot of other affected files, I've found 30+ including
/proc/net/ip_tables_matches and /proc/sysvipc/*

This patch-set fixes files related to tpm driver:
/sys/kernel/security/tpm0/ascii_bios_measurements
/sys/kernel/security/tpm0/binary_bios_measurements

https://bugzilla.kernel.org/show_bug.cgi?id=206283

Vasily Averin (2):
  tpm: tpm1_bios_measurements_next should increase position index
  tpm: tpm2_bios_measurements_next should increase position index

 drivers/char/tpm/eventlog/tpm1.c | 2 +-
 drivers/char/tpm/eventlog/tpm2.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

-- 
1.8.3.1

[PATCH v2 1/2] tpm: tpm1_bios_measurements_next should increase position index

[Vasily Averin]

If seq_file .next function does not change position index,
read after non-zero lseek can generate unexpected output.
In case of /sys/kernel/security/tpm0/ascii_bios_measurements
and binary_bios_measurements:
1) read after lseek beyond end of file generates whole last line.
2) read after lseek to middle of last line generates
expected end of last line and unexpected whole last line once again.

Cc: stable@vger.kernel.org
Fixes: 1f4aace60b0e ("fs/seq_file.c: simplify seq_file iteration code ...")
https://bugzilla.kernel.org/show_bug.cgi?id=206283
Signed-off-by: Vasily Averin <vvs@virtuozzo.com>
---
 drivers/char/tpm/eventlog/tpm1.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/char/tpm/eventlog/tpm1.c b/drivers/char/tpm/eventlog/tpm1.c
index 739b1d9..2c96977 100644
--- a/drivers/char/tpm/eventlog/tpm1.c
+++ b/drivers/char/tpm/eventlog/tpm1.c
@@ -115,6 +115,7 @@ static void *tpm1_bios_measurements_next(struct seq_file *m, void *v,
 	u32 converted_event_size;
 	u32 converted_event_type;
 
+	(*pos)++;
 	converted_event_size = do_endian_conversion(event->event_size);
 
 	v += sizeof(struct tcpa_event) + converted_event_size;
@@ -132,7 +133,6 @@ static void *tpm1_bios_measurements_next(struct seq_file *m, void *v,
 	    ((v + sizeof(struct tcpa_event) + converted_event_size) > limit))
 		return NULL;
 
-	(*pos)++;
 	return v;
 }
 
-- 
1.8.3.1

[PATCH v2 2/2] tpm: tpm2_bios_measurements_next should increase position index

[Vasily Averin]

If seq_file .next function does not change position index,
read after non-zero lseek can generate unexpected output.
For /sys/kernel/security/tpm0/binary_bios_measurements:
1) read after lseek beyond end of file generates whole last line.
2) read after lseek to middle of last line generates
expected end of last line and unexpected whole last line once again.

Cc: stable@vger.kernel.org
Fixes: 1f4aace60b0e ("fs/seq_file.c: simplify seq_file iteration code ...")

https://bugzilla.kernel.org/show_bug.cgi?id=206283
Signed-off-by: Vasily Averin <vvs@virtuozzo.com>
---
 drivers/char/tpm/eventlog/tpm2.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/char/tpm/eventlog/tpm2.c b/drivers/char/tpm/eventlog/tpm2.c
index b9aeda1..e741b11 100644
--- a/drivers/char/tpm/eventlog/tpm2.c
+++ b/drivers/char/tpm/eventlog/tpm2.c
@@ -94,6 +94,7 @@ static void *tpm2_bios_measurements_next(struct seq_file *m, void *v,
 	size_t event_size;
 	void *marker;
 
+	(*pos)++;
 	event_header = log->bios_event_log;
 
 	if (v == SEQ_START_TOKEN) {
@@ -118,7 +119,6 @@ static void *tpm2_bios_measurements_next(struct seq_file *m, void *v,
 	if (((v + event_size) >= limit) || (event_size == 0))
 		return NULL;
 
-	(*pos)++;
 	return v;
 }
 
-- 
1.8.3.1

Re: [PATCH v2 2/2] tpm: tpm2_bios_measurements_next should increase position index

[Jarkko Sakkinen]

On Thu, Jan 30, 2020 at 01:23:21PM +0300, Vasily Averin wrote:
> If seq_file .next function does not change position index,
> read after non-zero lseek can generate unexpected output.

Is it unwanted or unexpected? Unexpected would be mean random
output. I don't think that is the case. Please describe more
throughly.

> For /sys/kernel/security/tpm0/binary_bios_measurements:
> 1) read after lseek beyond end of file generates whole last line.
> 2) read after lseek to middle of last line generates
> expected end of last line and unexpected whole last line once again.
> 
> Cc: stable@vger.kernel.org
> Fixes: 1f4aace60b0e ("fs/seq_file.c: simplify seq_file iteration code ...")
> 
No empty line here.

> https://bugzilla.kernel.org/show_bug.cgi?id=206283

"Link: https://bugzilla.kernel.org/show_bug.cgi?id=206283"

/Jarkko

Re: [PATCH v2 2/2] tpm: tpm2_bios_measurements_next should increase position index

[Vasily Averin]

On 2/1/20 8:03 PM, Jarkko Sakkinen wrote:
> On Thu, Jan 30, 2020 at 01:23:21PM +0300, Vasily Averin wrote:
>> If seq_file .next function does not change position index,
>> read after non-zero lseek can generate unexpected output.
> 
> Is it unwanted or unexpected? Unexpected would be mean random
> output. I don't think that is the case. Please describe more
> throughly.

If .next function does not change position index, 
following .show function will repeat output related to current position index.

>> For /sys/kernel/security/tpm0/binary_bios_measurements:
>> 1) read after lseek beyond end of file generates whole last line.
>> 2) read after lseek to middle of last line generates
>> expected end of last line and unexpected whole last line once again.
>>
>> Cc: stable@vger.kernel.org
>> Fixes: 1f4aace60b0e ("fs/seq_file.c: simplify seq_file iteration code ...")
>>
> No empty line here.
> 
>> https://bugzilla.kernel.org/show_bug.cgi?id=206283
> 
> "Link: https://bugzilla.kernel.org/show_bug.cgi?id=206283"
> 
> /Jarkko
> 

Re: [PATCH v2 2/2] tpm: tpm2_bios_measurements_next should increase position index

[Jarkko Sakkinen]

On Mon, Feb 03, 2020 at 08:14:53AM +0300, Vasily Averin wrote:
> On 2/1/20 8:03 PM, Jarkko Sakkinen wrote:
> > On Thu, Jan 30, 2020 at 01:23:21PM +0300, Vasily Averin wrote:
> >> If seq_file .next function does not change position index,
> >> read after non-zero lseek can generate unexpected output.
> > 
> > Is it unwanted or unexpected? Unexpected would be mean random
> > output. I don't think that is the case. Please describe more
> > throughly.
> 
> If .next function does not change position index, 
> following .show function will repeat output related to current position index.

Thank you. That is clear and concise.

Then, please put that to the commit message instead of "unexpected
output".

/Jarkko

[PATCH v3 1/2] tpm: tpm1_bios_measurements_next should increase position index

[Vasily Averin]

If .next function does not change position index,
following .show function will repeat output related
to current position index.

In case of /sys/kernel/security/tpm0/ascii_bios_measurements
and binary_bios_measurements:
1) read after lseek beyound end of file generates whole last line.
2) read after lseek to middle of last line generates
expected end of last line and unexpected whole last line once again.

Cc: stable@vger.kernel.org
Fixes: 1f4aace60b0e ("fs/seq_file.c: simplify seq_file iteration code ...")
Link: https://bugzilla.kernel.org/show_bug.cgi?id=206283
Signed-off-by: Vasily Averin <vvs@virtuozzo.com>
---
 drivers/char/tpm/eventlog/tpm1.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/char/tpm/eventlog/tpm1.c b/drivers/char/tpm/eventlog/tpm1.c
index 739b1d9..2c96977 100644
--- a/drivers/char/tpm/eventlog/tpm1.c
+++ b/drivers/char/tpm/eventlog/tpm1.c
@@ -115,6 +115,7 @@ static void *tpm1_bios_measurements_next(struct seq_file *m, void *v,
 	u32 converted_event_size;
 	u32 converted_event_type;
 
+	(*pos)++;
 	converted_event_size = do_endian_conversion(event->event_size);
 
 	v += sizeof(struct tcpa_event) + converted_event_size;
@@ -132,7 +133,6 @@ static void *tpm1_bios_measurements_next(struct seq_file *m, void *v,
 	    ((v + sizeof(struct tcpa_event) + converted_event_size) > limit))
 		return NULL;
 
-	(*pos)++;
 	return v;
 }
 
-- 
1.8.3.1

[PATCH v3 2/2] tpm: tpm2_bios_measurements_next should increase position index

[Vasily Averin]

If .next function does not change position index,
following .show function will repeat output related
to current position index.

For /sys/kernel/security/tpm0/binary_bios_measurements:
1) read after lseek beyound end of file generates whole last line.
2) read after lseek to middle of last line generates
expected end of last line and unexpected whole last line once again.

Cc: stable@vger.kernel.org
Fixes: 1f4aace60b0e ("fs/seq_file.c: simplify seq_file iteration code ...")
Link: https://bugzilla.kernel.org/show_bug.cgi?id=206283
Signed-off-by: Vasily Averin <vvs@virtuozzo.com>
---
 drivers/char/tpm/eventlog/tpm2.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/char/tpm/eventlog/tpm2.c b/drivers/char/tpm/eventlog/tpm2.c
index b9aeda1..e741b11 100644
--- a/drivers/char/tpm/eventlog/tpm2.c
+++ b/drivers/char/tpm/eventlog/tpm2.c
@@ -94,6 +94,7 @@ static void *tpm2_bios_measurements_next(struct seq_file *m, void *v,
 	size_t event_size;
 	void *marker;
 
+	(*pos)++;
 	event_header = log->bios_event_log;
 
 	if (v == SEQ_START_TOKEN) {
@@ -118,7 +119,6 @@ static void *tpm2_bios_measurements_next(struct seq_file *m, void *v,
 	if (((v + event_size) >= limit) || (event_size == 0))
 		return NULL;
 
-	(*pos)++;
 	return v;
 }
 
-- 
1.8.3.1

Re: [PATCH v3 1/2] tpm: tpm1_bios_measurements_next should increase position index

[Jarkko Sakkinen]

On Tue, Feb 25, 2020 at 09:26:08AM +0300, Vasily Averin wrote:
> If .next function does not change position index,
> following .show function will repeat output related
> to current position index.
> 
> In case of /sys/kernel/security/tpm0/ascii_bios_measurements
> and binary_bios_measurements:
> 1) read after lseek beyound end of file generates whole last line.
> 2) read after lseek to middle of last line generates
> expected end of last line and unexpected whole last line once again.
> 
> Cc: stable@vger.kernel.org
> Fixes: 1f4aace60b0e ("fs/seq_file.c: simplify seq_file iteration code ...")
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=206283
> Signed-off-by: Vasily Averin <vvs@virtuozzo.com>

Thank alot!

Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>

/Jarkko

Re: [PATCH v3 1/2] tpm: tpm1_bios_measurements_next should increase position index

[Jarkko Sakkinen]

On Tue, Feb 25, 2020 at 12:27:59PM +0200, Jarkko Sakkinen wrote:
> On Tue, Feb 25, 2020 at 09:26:08AM +0300, Vasily Averin wrote:
> > If .next function does not change position index,
> > following .show function will repeat output related
> > to current position index.
> > 
> > In case of /sys/kernel/security/tpm0/ascii_bios_measurements
> > and binary_bios_measurements:
> > 1) read after lseek beyound end of file generates whole last line.
> > 2) read after lseek to middle of last line generates
> > expected end of last line and unexpected whole last line once again.
> > 
> > Cc: stable@vger.kernel.org
> > Fixes: 1f4aace60b0e ("fs/seq_file.c: simplify seq_file iteration code ...")
> > Link: https://bugzilla.kernel.org/show_bug.cgi?id=206283
> > Signed-off-by: Vasily Averin <vvs@virtuozzo.com>
> 
> Thank alot!
> 
> Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>

Applied both.

I'll try to squeeze these to v5.6.

/Jarkko