Patchwork [PATCHv2,7/9] UBIFS: do not write rubbish into truncation scanning node

login
register
mail settings
Submitter Artem Bityutskiy
Date Aug. 8, 2010, 9:57 a.m.
Message ID <1281261452-9868-8-git-send-email-dedekind1@gmail.com>
Download mbox | patch
Permalink /patch/61201/
State Accepted
Commit 5b7a3a2e1b0cbc7d5410a8da60dac266a3e19268
Headers show

Comments

Artem Bityutskiy - Aug. 8, 2010, 9:57 a.m.
From: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>

In the scanning code, in 'ubifs_add_snod()', we write rubbish into
'snod->key', because we assume that on-flash truncation nodes have a key, but
they do not. If the other parts of UBIFS then mistakenly try to look-up
the truncation node key (they should not do this, but may do because of a bug),
we can succeed and corrupt TNC. It looks like we did have such a situation in
'sort_nodes()' in gc.c.

Signed-off-by: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
---
 fs/ubifs/scan.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)
Adrian Hunter - Aug. 8, 2010, 11:03 a.m.
Artem Bityutskiy wrote:
> From: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
> 
> In the scanning code, in 'ubifs_add_snod()', we write rubbish into
> 'snod->key', because we assume that on-flash truncation nodes have a key, but
> they do not. If the other parts of UBIFS then mistakenly try to look-up
> the truncation node key (they should not do this, but may do because of a bug),
> we can succeed and corrupt TNC. It looks like we did have such a situation in
> 'sort_nodes()' in gc.c.
> 
> Signed-off-by: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
> ---
>  fs/ubifs/scan.c |    1 -
>  1 files changed, 0 insertions(+), 1 deletions(-)
> 
> diff --git a/fs/ubifs/scan.c b/fs/ubifs/scan.c
> index 96c5253..a0a305c 100644
> --- a/fs/ubifs/scan.c
> +++ b/fs/ubifs/scan.c
> @@ -212,7 +212,6 @@ int ubifs_add_snod(const struct ubifs_info *c, struct ubifs_scan_leb *sleb,
>  	case UBIFS_DENT_NODE:
>  	case UBIFS_XENT_NODE:
>  	case UBIFS_DATA_NODE:
> -	case UBIFS_TRUN_NODE:

There is another bug in ubifs_recover_size_accum() that expects a truncate node
to have a key.  That would be fixed by using trun_key_init() here.

>  		/*
>  		 * The key is in the same place in all keyed
>  		 * nodes.
Adrian Hunter - Aug. 8, 2010, 11:08 a.m.
Adrian Hunter wrote:
> Artem Bityutskiy wrote:
>> From: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
>>
>> In the scanning code, in 'ubifs_add_snod()', we write rubbish into
>> 'snod->key', because we assume that on-flash truncation nodes have a key, but
>> they do not. If the other parts of UBIFS then mistakenly try to look-up
>> the truncation node key (they should not do this, but may do because of a bug),
>> we can succeed and corrupt TNC. It looks like we did have such a situation in
>> 'sort_nodes()' in gc.c.
>>
>> Signed-off-by: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
>> ---
>>  fs/ubifs/scan.c |    1 -
>>  1 files changed, 0 insertions(+), 1 deletions(-)
>>
>> diff --git a/fs/ubifs/scan.c b/fs/ubifs/scan.c
>> index 96c5253..a0a305c 100644
>> --- a/fs/ubifs/scan.c
>> +++ b/fs/ubifs/scan.c
>> @@ -212,7 +212,6 @@ int ubifs_add_snod(const struct ubifs_info *c, struct ubifs_scan_leb *sleb,
>>  	case UBIFS_DENT_NODE:
>>  	case UBIFS_XENT_NODE:
>>  	case UBIFS_DATA_NODE:
>> -	case UBIFS_TRUN_NODE:
> 
> There is another bug in ubifs_recover_size_accum() that expects a truncate node
> to have a key.  That would be fixed by using trun_key_init() here.

No I was wrong, the key in ubifs_recover_size_accum() is already created using
trun_key_init()

Still, trun_key_init might be better here because the all-zeroes key has a key-type
of UBIFS_INO_KEY.  What do you think?

> 
>>  		/*
>>  		 * The key is in the same place in all keyed
>>  		 * nodes.
> 
>
Artem Bityutskiy - Aug. 8, 2010, 1:59 p.m.
On Sun, 2010-08-08 at 14:08 +0300, Adrian Hunter wrote:
> No I was wrong, the key in ubifs_recover_size_accum() is already created using
> trun_key_init()
> 
> Still, trun_key_init might be better here because the all-zeroes key has a key-type
> of UBIFS_INO_KEY.  What do you think?

Did not really think, but off the top of my head it is probably good to
initialize ->key to something invalid of all other nodes then.

Patch

diff --git a/fs/ubifs/scan.c b/fs/ubifs/scan.c
index 96c5253..a0a305c 100644
--- a/fs/ubifs/scan.c
+++ b/fs/ubifs/scan.c
@@ -212,7 +212,6 @@  int ubifs_add_snod(const struct ubifs_info *c, struct ubifs_scan_leb *sleb,
 	case UBIFS_DENT_NODE:
 	case UBIFS_XENT_NODE:
 	case UBIFS_DATA_NODE:
-	case UBIFS_TRUN_NODE:
 		/*
 		 * The key is in the same place in all keyed
 		 * nodes.