* [PATCH] reiserfs: fix broken xattr handling (heap corruption, bad retval)
@ 2018-08-02 15:15 Jann Horn
2018-08-10 3:19 ` Jann Horn
2018-08-13 18:39 ` Jeff Mahoney
0 siblings, 2 replies; 6+ messages in thread
From: Jann Horn @ 2018-08-02 15:15 UTC (permalink / raw)
To: reiserfs-devel, Andrew Morton, jannh
Cc: linux-kernel, Jeff Mahoney, Eric Biggers, Al Viro
This fixes the following issues:
- When a buffer size is supplied to reiserfs_listxattr() such that each
individual name fits, but the concatenation of all names doesn't
fit, reiserfs_listxattr() overflows the supplied buffer. This leads to
a kernel heap overflow (verified using KASAN) followed by an
out-of-bounds usercopy and is therefore a security bug.
- When a buffer size is supplied to reiserfs_listxattr() such that a name
doesn't fit, -ERANGE should be returned. But reiserfs instead just
truncates the list of names; I have verified that if the only xattr on
a file has a longer name than the supplied buffer length, listxattr()
incorrectly returns zero.
With my patch applied, -ERANGE is returned in both cases and the memory
corruption doesn't happen anymore.
Credit for making me clean this code up a bit goes to Al Viro, who pointed
out that the ->actor calling convention is suboptimal and should be
changed.
Fixes: 48b32a3553a5 ("reiserfs: use generic xattr handlers")
Cc: stable@vger.kernel.org
Signed-off-by: Jann Horn <jannh@google.com>
---
Triggering the bug:
root@debian:/home/user# mount -o user_xattr reiserimg reisermount/
root@debian:/home/user# cd reisermount/
root@debian:/home/user/reisermount# touch test_file
root@debian:/home/user/reisermount# setfattr -n user.foo1 -v A test_file
root@debian:/home/user/reisermount# setfattr -n user.foo2 -v A test_file
root@debian:/home/user/reisermount# setfattr -n user.foo3 -v A test_file
root@debian:/home/user/reisermount# setfattr -n user.foo4 -v A test_file
root@debian:/home/user/reisermount# setfattr -n user.foo5 -v A test_file
root@debian:/home/user/reisermount# setfattr -n user.foo6 -v A test_file
root@debian:/home/user/reisermount# cat xattr_test.c
#include <sys/types.h>
#include <attr/xattr.h>
#include <err.h>
#include <stdio.h>
#include <string.h>
int main(int argc, char **argv) {
if (argc != 2) errx(1, "bad invocation");
char list[10];
int res = listxattr(argv[1], list, sizeof(list));
if (res == -1)
err(1, "listxattr failed");
printf("listxattr returned %d\n", res);
for (char *p = list; p < list+res-1; p = p + strlen(p) + 1) {
printf("list entry: %s\n", p);
}
}
root@debian:/home/user/reisermount# gcc -o xattr_test xattr_test.c
root@debian:/home/user/reisermount# ./xattr_test test_file
Segmentation fault
root@debian:/home/user/reisermount#
Result:
[ 122.071318] ==================================================================
[ 122.072334] BUG: KASAN: slab-out-of-bounds in listxattr_filler+0x170/0x1b0
[ 122.073173] Write of size 9 at addr ffff8801c43b474a by task xattr_test/923
[ 122.074030]
[ 122.074223] CPU: 1 PID: 923 Comm: xattr_test Not tainted 4.18.0-rc7+ #67
[ 122.075050] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.2-1 04/01/2014
[ 122.076107] Call Trace:
[ 122.076453] dump_stack+0x71/0xab
[ 122.076900] print_address_description+0x6a/0x250
[ 122.077514] kasan_report+0x258/0x380
[ 122.077961] ? listxattr_filler+0x170/0x1b0
[ 122.078469] memcpy+0x34/0x50
[ 122.078894] listxattr_filler+0x170/0x1b0
[...]
fs/reiserfs/xattr.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/fs/reiserfs/xattr.c b/fs/reiserfs/xattr.c
index ff94fad477e4..48cdfc81fe10 100644
--- a/fs/reiserfs/xattr.c
+++ b/fs/reiserfs/xattr.c
@@ -792,8 +792,10 @@ static int listxattr_filler(struct dir_context *ctx, const char *name,
return 0;
size = namelen + 1;
if (b->buf) {
- if (size > b->size)
+ if (b->pos + size > b->size) {
+ b->pos = -ERANGE;
return -ERANGE;
+ }
memcpy(b->buf + b->pos, name, namelen);
b->buf[b->pos + namelen] = 0;
}
--
2.18.0.597.ga71716f1ad-goog
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] reiserfs: fix broken xattr handling (heap corruption, bad retval)
2018-08-02 15:15 [PATCH] reiserfs: fix broken xattr handling (heap corruption, bad retval) Jann Horn
@ 2018-08-10 3:19 ` Jann Horn
2018-08-13 17:42 ` Will Deacon
2018-08-13 18:39 ` Jeff Mahoney
1 sibling, 1 reply; 6+ messages in thread
From: Jann Horn @ 2018-08-10 3:19 UTC (permalink / raw)
To: reiserfs-devel, Andrew Morton, security, Al Viro, jeffm
Cc: kernel list, ebiggers
On Thu, Aug 2, 2018 at 5:16 PM Jann Horn <jannh@google.com> wrote:
>
> This fixes the following issues:
>
> - When a buffer size is supplied to reiserfs_listxattr() such that each
> individual name fits, but the concatenation of all names doesn't
> fit, reiserfs_listxattr() overflows the supplied buffer. This leads to
> a kernel heap overflow (verified using KASAN) followed by an
> out-of-bounds usercopy and is therefore a security bug.
> - When a buffer size is supplied to reiserfs_listxattr() such that a name
> doesn't fit, -ERANGE should be returned. But reiserfs instead just
> truncates the list of names; I have verified that if the only xattr on
> a file has a longer name than the supplied buffer length, listxattr()
> incorrectly returns zero.
>
> With my patch applied, -ERANGE is returned in both cases and the memory
> corruption doesn't happen anymore.
>
> Credit for making me clean this code up a bit goes to Al Viro, who pointed
> out that the ->actor calling convention is suboptimal and should be
> changed.
>
> Fixes: 48b32a3553a5 ("reiserfs: use generic xattr handlers")
> Cc: stable@vger.kernel.org
> Signed-off-by: Jann Horn <jannh@google.com>
+security@
Ping. I have not received any replies to this patch, which fixes a
kernel security bug, for a week.
Whose tree should this go through? reiserfs is marked as "supported",
but does not have a maintainer or a git repo listed, just a
mailinglist, so I guess it probably has to go through either Al Viro's
or akpm's tree? Looks like akpm signed off on the last commits in
reiserfs...
> ---
> Triggering the bug:
>
> root@debian:/home/user# mount -o user_xattr reiserimg reisermount/
> root@debian:/home/user# cd reisermount/
> root@debian:/home/user/reisermount# touch test_file
> root@debian:/home/user/reisermount# setfattr -n user.foo1 -v A test_file
> root@debian:/home/user/reisermount# setfattr -n user.foo2 -v A test_file
> root@debian:/home/user/reisermount# setfattr -n user.foo3 -v A test_file
> root@debian:/home/user/reisermount# setfattr -n user.foo4 -v A test_file
> root@debian:/home/user/reisermount# setfattr -n user.foo5 -v A test_file
> root@debian:/home/user/reisermount# setfattr -n user.foo6 -v A test_file
> root@debian:/home/user/reisermount# cat xattr_test.c
> #include <sys/types.h>
> #include <attr/xattr.h>
> #include <err.h>
> #include <stdio.h>
> #include <string.h>
> int main(int argc, char **argv) {
> if (argc != 2) errx(1, "bad invocation");
> char list[10];
> int res = listxattr(argv[1], list, sizeof(list));
> if (res == -1)
> err(1, "listxattr failed");
> printf("listxattr returned %d\n", res);
> for (char *p = list; p < list+res-1; p = p + strlen(p) + 1) {
> printf("list entry: %s\n", p);
> }
> }
> root@debian:/home/user/reisermount# gcc -o xattr_test xattr_test.c
> root@debian:/home/user/reisermount# ./xattr_test test_file
> Segmentation fault
> root@debian:/home/user/reisermount#
>
> Result:
>
> [ 122.071318] ==================================================================
> [ 122.072334] BUG: KASAN: slab-out-of-bounds in listxattr_filler+0x170/0x1b0
> [ 122.073173] Write of size 9 at addr ffff8801c43b474a by task xattr_test/923
> [ 122.074030]
> [ 122.074223] CPU: 1 PID: 923 Comm: xattr_test Not tainted 4.18.0-rc7+ #67
> [ 122.075050] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.2-1 04/01/2014
> [ 122.076107] Call Trace:
> [ 122.076453] dump_stack+0x71/0xab
> [ 122.076900] print_address_description+0x6a/0x250
> [ 122.077514] kasan_report+0x258/0x380
> [ 122.077961] ? listxattr_filler+0x170/0x1b0
> [ 122.078469] memcpy+0x34/0x50
> [ 122.078894] listxattr_filler+0x170/0x1b0
> [...]
>
> fs/reiserfs/xattr.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/fs/reiserfs/xattr.c b/fs/reiserfs/xattr.c
> index ff94fad477e4..48cdfc81fe10 100644
> --- a/fs/reiserfs/xattr.c
> +++ b/fs/reiserfs/xattr.c
> @@ -792,8 +792,10 @@ static int listxattr_filler(struct dir_context *ctx, const char *name,
> return 0;
> size = namelen + 1;
> if (b->buf) {
> - if (size > b->size)
> + if (b->pos + size > b->size) {
> + b->pos = -ERANGE;
> return -ERANGE;
> + }
> memcpy(b->buf + b->pos, name, namelen);
> b->buf[b->pos + namelen] = 0;
> }
> --
> 2.18.0.597.ga71716f1ad-goog
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] reiserfs: fix broken xattr handling (heap corruption, bad retval)
2018-08-10 3:19 ` Jann Horn
@ 2018-08-13 17:42 ` Will Deacon
2018-08-13 18:04 ` Jann Horn
0 siblings, 1 reply; 6+ messages in thread
From: Will Deacon @ 2018-08-13 17:42 UTC (permalink / raw)
To: Jann Horn
Cc: reiserfs-devel, Andrew Morton, security, Al Viro, jeffm,
kernel list, ebiggers
Hi Jann,
On Fri, Aug 10, 2018 at 05:19:38AM +0200, Jann Horn wrote:
> On Thu, Aug 2, 2018 at 5:16 PM Jann Horn <jannh@google.com> wrote:
> >
> > This fixes the following issues:
> >
> > - When a buffer size is supplied to reiserfs_listxattr() such that each
> > individual name fits, but the concatenation of all names doesn't
> > fit, reiserfs_listxattr() overflows the supplied buffer. This leads to
> > a kernel heap overflow (verified using KASAN) followed by an
> > out-of-bounds usercopy and is therefore a security bug.
> > - When a buffer size is supplied to reiserfs_listxattr() such that a name
> > doesn't fit, -ERANGE should be returned. But reiserfs instead just
> > truncates the list of names; I have verified that if the only xattr on
> > a file has a longer name than the supplied buffer length, listxattr()
> > incorrectly returns zero.
> >
> > With my patch applied, -ERANGE is returned in both cases and the memory
> > corruption doesn't happen anymore.
> >
> > Credit for making me clean this code up a bit goes to Al Viro, who pointed
> > out that the ->actor calling convention is suboptimal and should be
> > changed.
> >
> > Fixes: 48b32a3553a5 ("reiserfs: use generic xattr handlers")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Jann Horn <jannh@google.com>
>
> +security@
> Ping. I have not received any replies to this patch, which fixes a
> kernel security bug, for a week.
> Whose tree should this go through? reiserfs is marked as "supported",
> but does not have a maintainer or a git repo listed, just a
> mailinglist, so I guess it probably has to go through either Al Viro's
> or akpm's tree? Looks like akpm signed off on the last commits in
> reiserfs...
I think Andrew's tree makes the most sense for this, but perhaps we should
also patch MAINTAINERS so mark it as "Orphan"? Patch below.
Will
--->8
From 07fbb021d5bbfe623fad10073b55704bda8e1f3d Mon Sep 17 00:00:00 2001
From: Will Deacon <will.deacon@arm.com>
Date: Mon, 13 Aug 2018 18:31:50 +0100
Subject: [PATCH] MAINTAINERS: Mark reiserfs as Orphan
Reiserfs has no Maintainer and random fixes tend to be merged through
with Andrew or Al's tree. Demote the filesystem to "Orphan", since it's
clear no longer supported by anybody.
Reported-by: Jann Horn <jannh@google.com>
Signed-off-by: Will Deacon <will.deacon@arm.com>
---
MAINTAINERS | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/MAINTAINERS b/MAINTAINERS
index 544cac829cf4..b4fcc19cfb52 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12077,7 +12077,7 @@ F: include/linux/regmap.h
REISERFS FILE SYSTEM
L: reiserfs-devel@vger.kernel.org
-S: Supported
+S: Orphan
F: fs/reiserfs/
REMOTE PROCESSOR (REMOTEPROC) SUBSYSTEM
--
2.1.4
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] reiserfs: fix broken xattr handling (heap corruption, bad retval)
2018-08-13 17:42 ` Will Deacon
@ 2018-08-13 18:04 ` Jann Horn
2018-08-13 18:39 ` Jeff Mahoney
0 siblings, 1 reply; 6+ messages in thread
From: Jann Horn @ 2018-08-13 18:04 UTC (permalink / raw)
To: Will Deacon, Jeff Mahoney
Cc: reiserfs-devel, Andrew Morton, security, Al Viro, kernel list,
Eric Biggers
On Mon, Aug 13, 2018 at 7:42 PM Will Deacon <will.deacon@arm.com> wrote:
>
> Hi Jann,
>
> On Fri, Aug 10, 2018 at 05:19:38AM +0200, Jann Horn wrote:
> > On Thu, Aug 2, 2018 at 5:16 PM Jann Horn <jannh@google.com> wrote:
> > >
> > > This fixes the following issues:
> > >
> > > - When a buffer size is supplied to reiserfs_listxattr() such that each
> > > individual name fits, but the concatenation of all names doesn't
> > > fit, reiserfs_listxattr() overflows the supplied buffer. This leads to
> > > a kernel heap overflow (verified using KASAN) followed by an
> > > out-of-bounds usercopy and is therefore a security bug.
> > > - When a buffer size is supplied to reiserfs_listxattr() such that a name
> > > doesn't fit, -ERANGE should be returned. But reiserfs instead just
> > > truncates the list of names; I have verified that if the only xattr on
> > > a file has a longer name than the supplied buffer length, listxattr()
> > > incorrectly returns zero.
> > >
> > > With my patch applied, -ERANGE is returned in both cases and the memory
> > > corruption doesn't happen anymore.
> > >
> > > Credit for making me clean this code up a bit goes to Al Viro, who pointed
> > > out that the ->actor calling convention is suboptimal and should be
> > > changed.
> > >
> > > Fixes: 48b32a3553a5 ("reiserfs: use generic xattr handlers")
> > > Cc: stable@vger.kernel.org
> > > Signed-off-by: Jann Horn <jannh@google.com>
> >
> > +security@
> > Ping. I have not received any replies to this patch, which fixes a
> > kernel security bug, for a week.
> > Whose tree should this go through? reiserfs is marked as "supported",
> > but does not have a maintainer or a git repo listed, just a
> > mailinglist, so I guess it probably has to go through either Al Viro's
> > or akpm's tree? Looks like akpm signed off on the last commits in
> > reiserfs...
>
> I think Andrew's tree makes the most sense for this,
Yeah, Andrew has already merged it. :)
http://ozlabs.org/~akpm/mmots/broken-out/reiserfs-fix-broken-xattr-handling-heap-corruption-bad-retval.patch
> but perhaps we should
> also patch MAINTAINERS so mark it as "Orphan"? Patch below.
Either that, or get someone to step up as maintainer? If I read
https://marc.info/?l=reiserfs-devel&m=153214303506948&w=2#0 correctly,
there's still an intent to fix things in reiserfs, even though no
maintainer is listed. (Jeff Mahoney, who wrote that message and is
CC'ed on this thread, seems to have been out of office last week - when
I sent the "Ping" message a few days ago, I got a vacation
autoresponder "I'll be out of the office until 13 August" from him.)
> Will
>
> --->8
>
> From 07fbb021d5bbfe623fad10073b55704bda8e1f3d Mon Sep 17 00:00:00 2001
> From: Will Deacon <will.deacon@arm.com>
> Date: Mon, 13 Aug 2018 18:31:50 +0100
> Subject: [PATCH] MAINTAINERS: Mark reiserfs as Orphan
>
> Reiserfs has no Maintainer and random fixes tend to be merged through
> with Andrew or Al's tree. Demote the filesystem to "Orphan", since it's
> clear no longer supported by anybody.
>
> Reported-by: Jann Horn <jannh@google.com>
> Signed-off-by: Will Deacon <will.deacon@arm.com>
> ---
> MAINTAINERS | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 544cac829cf4..b4fcc19cfb52 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -12077,7 +12077,7 @@ F: include/linux/regmap.h
>
> REISERFS FILE SYSTEM
> L: reiserfs-devel@vger.kernel.org
> -S: Supported
> +S: Orphan
> F: fs/reiserfs/
>
> REMOTE PROCESSOR (REMOTEPROC) SUBSYSTEM
> --
> 2.1.4
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] reiserfs: fix broken xattr handling (heap corruption, bad retval)
2018-08-13 18:04 ` Jann Horn
@ 2018-08-13 18:39 ` Jeff Mahoney
0 siblings, 0 replies; 6+ messages in thread
From: Jeff Mahoney @ 2018-08-13 18:39 UTC (permalink / raw)
To: Jann Horn, Will Deacon
Cc: reiserfs-devel, Andrew Morton, security, Al Viro, kernel list,
Eric Biggers
[-- Attachment #1.1: Type: text/plain, Size: 3983 bytes --]
On 8/13/18 2:04 PM, Jann Horn wrote:
> On Mon, Aug 13, 2018 at 7:42 PM Will Deacon <will.deacon@arm.com> wrote:
>>
>> Hi Jann,
>>
>> On Fri, Aug 10, 2018 at 05:19:38AM +0200, Jann Horn wrote:
>>> On Thu, Aug 2, 2018 at 5:16 PM Jann Horn <jannh@google.com> wrote:
>>>>
>>>> This fixes the following issues:
>>>>
>>>> - When a buffer size is supplied to reiserfs_listxattr() such that each
>>>> individual name fits, but the concatenation of all names doesn't
>>>> fit, reiserfs_listxattr() overflows the supplied buffer. This leads to
>>>> a kernel heap overflow (verified using KASAN) followed by an
>>>> out-of-bounds usercopy and is therefore a security bug.
>>>> - When a buffer size is supplied to reiserfs_listxattr() such that a name
>>>> doesn't fit, -ERANGE should be returned. But reiserfs instead just
>>>> truncates the list of names; I have verified that if the only xattr on
>>>> a file has a longer name than the supplied buffer length, listxattr()
>>>> incorrectly returns zero.
>>>>
>>>> With my patch applied, -ERANGE is returned in both cases and the memory
>>>> corruption doesn't happen anymore.
>>>>
>>>> Credit for making me clean this code up a bit goes to Al Viro, who pointed
>>>> out that the ->actor calling convention is suboptimal and should be
>>>> changed.
>>>>
>>>> Fixes: 48b32a3553a5 ("reiserfs: use generic xattr handlers")
>>>> Cc: stable@vger.kernel.org
>>>> Signed-off-by: Jann Horn <jannh@google.com>
>>>
>>> +security@
>>> Ping. I have not received any replies to this patch, which fixes a
>>> kernel security bug, for a week.
>>> Whose tree should this go through? reiserfs is marked as "supported",
>>> but does not have a maintainer or a git repo listed, just a
>>> mailinglist, so I guess it probably has to go through either Al Viro's
>>> or akpm's tree? Looks like akpm signed off on the last commits in
>>> reiserfs...
>>
>> I think Andrew's tree makes the most sense for this,
>
> Yeah, Andrew has already merged it. :)
> http://ozlabs.org/~akpm/mmots/broken-out/reiserfs-fix-broken-xattr-handling-heap-corruption-bad-retval.patch
>
>> but perhaps we should
>> also patch MAINTAINERS so mark it as "Orphan"? Patch below.
>
> Either that, or get someone to step up as maintainer? If I read
> https://marc.info/?l=reiserfs-devel&m=153214303506948&w=2#0 correctly,
> there's still an intent to fix things in reiserfs, even though no
> maintainer is listed. (Jeff Mahoney, who wrote that message and is
> CC'ed on this thread, seems to have been out of office last week - when
> I sent the "Ping" message a few days ago, I got a vacation
> autoresponder "I'll be out of the office until 13 August" from him.)
I suppose I can take a more active role here. I'm probably the person
with the most experience with reiserfs who still has a role where I need
to care about it.
-Jeff
>> Will
>>
>> --->8
>>
>> From 07fbb021d5bbfe623fad10073b55704bda8e1f3d Mon Sep 17 00:00:00 2001
>> From: Will Deacon <will.deacon@arm.com>
>> Date: Mon, 13 Aug 2018 18:31:50 +0100
>> Subject: [PATCH] MAINTAINERS: Mark reiserfs as Orphan
>>
>> Reiserfs has no Maintainer and random fixes tend to be merged through
>> with Andrew or Al's tree. Demote the filesystem to "Orphan", since it's
>> clear no longer supported by anybody.
>>
>> Reported-by: Jann Horn <jannh@google.com>
>> Signed-off-by: Will Deacon <will.deacon@arm.com>
>> ---
>> MAINTAINERS | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 544cac829cf4..b4fcc19cfb52 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -12077,7 +12077,7 @@ F: include/linux/regmap.h
>>
>> REISERFS FILE SYSTEM
>> L: reiserfs-devel@vger.kernel.org
>> -S: Supported
>> +S: Orphan
>> F: fs/reiserfs/
>>
>> REMOTE PROCESSOR (REMOTEPROC) SUBSYSTEM
>> --
>> 2.1.4
>
--
Jeff Mahoney
SUSE Labs
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] reiserfs: fix broken xattr handling (heap corruption, bad retval)
2018-08-02 15:15 [PATCH] reiserfs: fix broken xattr handling (heap corruption, bad retval) Jann Horn
2018-08-10 3:19 ` Jann Horn
@ 2018-08-13 18:39 ` Jeff Mahoney
1 sibling, 0 replies; 6+ messages in thread
From: Jeff Mahoney @ 2018-08-13 18:39 UTC (permalink / raw)
To: Jann Horn, reiserfs-devel, Andrew Morton
Cc: linux-kernel, Eric Biggers, Al Viro
[-- Attachment #1.1: Type: text/plain, Size: 4100 bytes --]
On 8/2/18 11:15 AM, Jann Horn wrote:
> This fixes the following issues:
>
> - When a buffer size is supplied to reiserfs_listxattr() such that each
> individual name fits, but the concatenation of all names doesn't
> fit, reiserfs_listxattr() overflows the supplied buffer. This leads to
> a kernel heap overflow (verified using KASAN) followed by an
> out-of-bounds usercopy and is therefore a security bug.
> - When a buffer size is supplied to reiserfs_listxattr() such that a name
> doesn't fit, -ERANGE should be returned. But reiserfs instead just
> truncates the list of names; I have verified that if the only xattr on
> a file has a longer name than the supplied buffer length, listxattr()
> incorrectly returns zero.
>
> With my patch applied, -ERANGE is returned in both cases and the memory
> corruption doesn't happen anymore.
>
> Credit for making me clean this code up a bit goes to Al Viro, who pointed
> out that the ->actor calling convention is suboptimal and should be
> changed.
>
> Fixes: 48b32a3553a5 ("reiserfs: use generic xattr handlers")
> Cc: stable@vger.kernel.org
> Signed-off-by: Jann Horn <jannh@google.com>
Acked-by: Jeff Mahoney <jeffm@suse.com>
Thanks,
-Jeff
> ---
> Triggering the bug:
>
> root@debian:/home/user# mount -o user_xattr reiserimg reisermount/
> root@debian:/home/user# cd reisermount/
> root@debian:/home/user/reisermount# touch test_file
> root@debian:/home/user/reisermount# setfattr -n user.foo1 -v A test_file
> root@debian:/home/user/reisermount# setfattr -n user.foo2 -v A test_file
> root@debian:/home/user/reisermount# setfattr -n user.foo3 -v A test_file
> root@debian:/home/user/reisermount# setfattr -n user.foo4 -v A test_file
> root@debian:/home/user/reisermount# setfattr -n user.foo5 -v A test_file
> root@debian:/home/user/reisermount# setfattr -n user.foo6 -v A test_file
> root@debian:/home/user/reisermount# cat xattr_test.c
> #include <sys/types.h>
> #include <attr/xattr.h>
> #include <err.h>
> #include <stdio.h>
> #include <string.h>
> int main(int argc, char **argv) {
> if (argc != 2) errx(1, "bad invocation");
> char list[10];
> int res = listxattr(argv[1], list, sizeof(list));
> if (res == -1)
> err(1, "listxattr failed");
> printf("listxattr returned %d\n", res);
> for (char *p = list; p < list+res-1; p = p + strlen(p) + 1) {
> printf("list entry: %s\n", p);
> }
> }
> root@debian:/home/user/reisermount# gcc -o xattr_test xattr_test.c
> root@debian:/home/user/reisermount# ./xattr_test test_file
> Segmentation fault
> root@debian:/home/user/reisermount#
>
> Result:
>
> [ 122.071318] ==================================================================
> [ 122.072334] BUG: KASAN: slab-out-of-bounds in listxattr_filler+0x170/0x1b0
> [ 122.073173] Write of size 9 at addr ffff8801c43b474a by task xattr_test/923
> [ 122.074030]
> [ 122.074223] CPU: 1 PID: 923 Comm: xattr_test Not tainted 4.18.0-rc7+ #67
> [ 122.075050] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.2-1 04/01/2014
> [ 122.076107] Call Trace:
> [ 122.076453] dump_stack+0x71/0xab
> [ 122.076900] print_address_description+0x6a/0x250
> [ 122.077514] kasan_report+0x258/0x380
> [ 122.077961] ? listxattr_filler+0x170/0x1b0
> [ 122.078469] memcpy+0x34/0x50
> [ 122.078894] listxattr_filler+0x170/0x1b0
> [...]
>
> fs/reiserfs/xattr.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/fs/reiserfs/xattr.c b/fs/reiserfs/xattr.c
> index ff94fad477e4..48cdfc81fe10 100644
> --- a/fs/reiserfs/xattr.c
> +++ b/fs/reiserfs/xattr.c
> @@ -792,8 +792,10 @@ static int listxattr_filler(struct dir_context *ctx, const char *name,
> return 0;
> size = namelen + 1;
> if (b->buf) {
> - if (size > b->size)
> + if (b->pos + size > b->size) {
> + b->pos = -ERANGE;
> return -ERANGE;
> + }
> memcpy(b->buf + b->pos, name, namelen);
> b->buf[b->pos + namelen] = 0;
> }
>
--
Jeff Mahoney
SUSE Labs
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-08-13 18:39 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-08-02 15:15 [PATCH] reiserfs: fix broken xattr handling (heap corruption, bad retval) Jann Horn
2018-08-10 3:19 ` Jann Horn
2018-08-13 17:42 ` Will Deacon
2018-08-13 18:04 ` Jann Horn
2018-08-13 18:39 ` Jeff Mahoney
2018-08-13 18:39 ` Jeff Mahoney
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).