[PATCH][RFC] HPFS: Don't use pointer to out-of-scope array

[PATCH][RFC] HPFS: Don't use pointer to out-of-scope array

[Jesper Juhl]

Hi,

There's some suspicious code in fs/hpfs/ea.c::hpfs_read_ea().

First of all, in the 'while (pos < len)' loop a local char array 'ex' is 
defined and subsequently the pointer variable 'ea' is set to point to the 
array. Inside the loop we may jump to the 'indirect' label which is 
outside the loop scope. At the 'indirect' label 'ea' (which now points 
to a array that is no longer in scope) is dereferenced - that's not good. 
The patch below addresses that problem by moving the 'ex' array out of 
the loop scope and into function scope.
But please not that this patch is RFC, not intended to be applied just yet 
since I don't, yet, feel confident enough that I really know what's going 
on here.

Secondly I fail to see how this code inside the while loop can even work:

  if (hpfs_ea_read(s, a, ano, pos + 4, ea->namelen + 1 + (ea->indirect ? 8 : 0), ex + 4))

There is nothing initializing 'ex' which 'ea' points to, so 
'ea->namelen' and 'ea->indirect' are uninitialized.
Isn't this a problem?


Signed-off-by: Jesper Juhl <jj@chaosbits.net>
---
 ea.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/hpfs/ea.c b/fs/hpfs/ea.c
index 45e53d9..4d6a90f 100644
--- a/fs/hpfs/ea.c
+++ b/fs/hpfs/ea.c
@@ -76,6 +76,7 @@ int hpfs_read_ea(struct super_block *s, struct fnode *fnode, char *key,
 	unsigned pos;
 	int ano, len;
 	secno a;
+	char ex[4 + 255 + 1 + 8];
 	struct extended_attribute *ea;
 	struct extended_attribute *ea_end = fnode_end_ea(fnode);
 	for (ea = fnode_ea(fnode); ea < ea_end; ea = next_ea(ea))
@@ -93,7 +94,6 @@ int hpfs_read_ea(struct super_block *s, struct fnode *fnode, char *key,
 	ano = fnode->ea_anode;
 	pos = 0;
 	while (pos < len) {
-		char ex[4 + 255 + 1 + 8];
 		ea = (struct extended_attribute *)ex;
 		if (pos + 4 > len) {
 			hpfs_error(s, "EAs don't end correctly, %s %08x, len %08x",



-- 
Jesper Juhl <jj@chaosbits.net>            http://www.chaosbits.net/
Don't top-post http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please.

Re: [PATCH][RFC] HPFS: Don't use pointer to out-of-scope array

[Andreas Schwab]

Jesper Juhl <jj@chaosbits.net> writes:

> Secondly I fail to see how this code inside the while loop can even work:
>
>   if (hpfs_ea_read(s, a, ano, pos + 4, ea->namelen + 1 + (ea->indirect ? 8 : 0), ex + 4))
>
> There is nothing initializing 'ex' which 'ea' points to, so 
> 'ea->namelen' and 'ea->indirect' are uninitialized.

The preceding hpfs_ea_read should do it (it reads the first 4 bytes of
ex which aliases ea->namelen and ea->indirect).

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [PATCH][RFC] HPFS: Don't use pointer to out-of-scope array

[Jesper Juhl]

On Fri, 24 Dec 2010, Andreas Schwab wrote:

> Jesper Juhl <jj@chaosbits.net> writes:
> 
> > Secondly I fail to see how this code inside the while loop can even work:
> >
> >   if (hpfs_ea_read(s, a, ano, pos + 4, ea->namelen + 1 + (ea->indirect ? 8 : 0), ex + 4))
> >
> > There is nothing initializing 'ex' which 'ea' points to, so 
> > 'ea->namelen' and 'ea->indirect' are uninitialized.
> 
> The preceding hpfs_ea_read should do it (it reads the first 4 bytes of
> ex which aliases ea->namelen and ea->indirect).
> 
How I managed to miss that I don't know, but I did and you are correct. 
Thanks.


-- 
Jesper Juhl <jj@chaosbits.net>            http://www.chaosbits.net/
Don't top-post http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please.

Re: [PATCH][RFC] HPFS: Don't use pointer to out-of-scope array

[Jesper Juhl]

On Fri, 24 Dec 2010, Jesper Juhl wrote:

> On Fri, 24 Dec 2010, Andreas Schwab wrote:
> 
> > Jesper Juhl <jj@chaosbits.net> writes:
> > 
> > > Secondly I fail to see how this code inside the while loop can even work:
> > >
> > >   if (hpfs_ea_read(s, a, ano, pos + 4, ea->namelen + 1 + (ea->indirect ? 8 : 0), ex + 4))
> > >
> > > There is nothing initializing 'ex' which 'ea' points to, so 
> > > 'ea->namelen' and 'ea->indirect' are uninitialized.
> > 
> > The preceding hpfs_ea_read should do it (it reads the first 4 bytes of
> > ex which aliases ea->namelen and ea->indirect).
> > 
> How I managed to miss that I don't know, but I did and you are correct. 
> Thanks.
> 
But, this bit: "in the 'while (pos < len)' loop a local char array 'ex' is 
defined and subsequently the pointer variable 'ea' is set to point to the 
array. Inside the loop we may jump to the 'indirect' label which is 
outside the loop scope. At the 'indirect' label 'ea' (which now points 
to a array that is no longer in scope) is dereferenced - that's not good. 
The patch below addresses that problem by moving the 'ex' array out of 
the loop scope and into function scope."
I still believe to be correct and that's what the patch actually 
addresses, so can we agree that the patch makes sense and should be 
merged?

-- 
Jesper Juhl <jj@chaosbits.net>            http://www.chaosbits.net/
Don't top-post http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please.

Re: [PATCH][RFC] HPFS: Don't use pointer to out-of-scope array

[Jesper Juhl]

On Thu, 6 Jan 2011, Jesper Juhl wrote:

> On Fri, 24 Dec 2010, Jesper Juhl wrote:
> 
> > On Fri, 24 Dec 2010, Andreas Schwab wrote:
> > 
> > > Jesper Juhl <jj@chaosbits.net> writes:
> > > 
> > > > Secondly I fail to see how this code inside the while loop can even work:
> > > >
> > > >   if (hpfs_ea_read(s, a, ano, pos + 4, ea->namelen + 1 + (ea->indirect ? 8 : 0), ex + 4))
> > > >
> > > > There is nothing initializing 'ex' which 'ea' points to, so 
> > > > 'ea->namelen' and 'ea->indirect' are uninitialized.
> > > 
> > > The preceding hpfs_ea_read should do it (it reads the first 4 bytes of
> > > ex which aliases ea->namelen and ea->indirect).
> > > 
> > How I managed to miss that I don't know, but I did and you are correct. 
> > Thanks.
> > 
> But, this bit: "in the 'while (pos < len)' loop a local char array 'ex' is 
> defined and subsequently the pointer variable 'ea' is set to point to the 
> array. Inside the loop we may jump to the 'indirect' label which is 
> outside the loop scope. At the 'indirect' label 'ea' (which now points 
> to a array that is no longer in scope) is dereferenced - that's not good. 
> The patch below addresses that problem by moving the 'ex' array out of 
> the loop scope and into function scope."
> I still believe to be correct and that's what the patch actually 
> addresses, so can we agree that the patch makes sense and should be 
> merged?
> 
Ping? 

-- 
Jesper Juhl <jj@chaosbits.net>       http://www.chaosbits.net/
Don't top-post http://www.catb.org/jargon/html/T/top-post.html
Plain text mails only, please.