linux-kernel - Re: [UPDATED PATCH] fix memory corruption from misinterpreted bad_inode

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <459D76A1.6000904@redhat.com>
Date:	Thu, 04 Jan 2007 15:50:25 -0600
From:	Eric Sandeen <sandeen@...hat.com>
To:	Linus Torvalds <torvalds@...l.org>
CC:	Andrew Morton <akpm@...l.org>, Al Viro <viro@....linux.org.uk>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	Al Viro <viro@...iv.linux.org.uk>
Subject: Re: [UPDATED PATCH] fix memory corruption from misinterpreted bad_inode_ops
 return values

Linus Torvalds wrote:
> 
> On Thu, 4 Jan 2007, Eric Sandeen wrote:
>> Andrew Morton wrote:
>>
>>> btw, couldn't we fix this bug with a simple old
>>>
>>> --- a/fs/bad_inode.c~a
>>> +++ a/fs/bad_inode.c
>>> @@ -15,7 +15,7 @@
>>>  #include <linux/smp_lock.h>
>>>  #include <linux/namei.h>
>>>  
>>> -static int return_EIO(void)
>>> +static long return_EIO(void)
>>>  {
>>>  	return -EIO;
>>>  }
>>> _
>>>
>>> ?
>> What about ops that return loff_t (64 bits) on 32-bit arches and stuff
>> it into 2 registers....
> 
> Do we actually have cases where we cast to a different return value?

Today, via the void * function casts in the bad file/inode ops, in
effect yes.

static int return_EIO(void)
{
        return -EIO;
}

#define EIO_ERROR ((void *) (return_EIO))

...
        .listxattr      = EIO_ERROR,

but listxattr is supposed to return a ssize_t, which is 64 bits on some
platforms, and only 32 bits get filled in thanks to the (void *) cast.
So we wind up with something other than the return value we expect...

Andrew's long suggestion breaks things the other way, with 64-bit
returning ops on 32-bit arches which again only pick up the first 32
bits thanks to the (void *) cast.

If we're really happy with casting away the function arguments (which
are not -used- in the bad_foo ops anyway), then I'd maybe suggest going
back to my first try at this thing:

static int return_EIO_int(void)
{
	return -EIO;
}
#define EIO_ERROR_INT ((void *) (return_EIO_int))

static struct inode_operations bad_inode_ops =
{
	.create		= EIO_ERROR_INT,
...etc...

which is most like what we have today, except with specific return types.

-Eric
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/