This is the issue causing #664562 -- it seems to be a 64-bit kernel,
32-bit userland compatibility issue with structure sizes. GCC prior to
version 4.7 would place the filename in a different place and so the
kernel writing beyond the end of the structure was not noticed.
Simon-

----- Forwarded message from Stefan Richter <stef...@s5r6.in-berlin.de> -----

Date: Fri, 5 Oct 2012 20:32:20 +0200
From: Stefan Richter <stef...@s5r6.in-berlin.de>
To: Simon Kirby <s...@hostway.ca>
Cc: Kristian Hogsberg <k...@redhat.com>, linux1394-de...@lists.sourceforge.net
Subject: Re: libraw1394 corruption with new GCC
X-Mailer: Claws Mail 3.8.0 (GTK+ 2.24.10; x86_64-pc-linux-gnu)

(adding Cc: linux1394-devel, quoting in full)

On Oct 05 Simon Kirby wrote:
> Hello! :)
> 
> I'm writing you as I'm trying to figure out some corruption that seems to
> be causing problems for me since GCC 4.7 started being used on compiled
> libraw1394 on Debian sid (i386). If I compile "fw.c" from this package
> with GCC 4.6, or with -fno-stack-protector, it works; otherwise,
> filename[] in the fw_set_port() function seems to get clobbered by this
> ioctl call:
> 
>       if (ioctl(fd, FW_CDEV_IOC_GET_INFO, &get_info) < 0) {
> 
> ...you seem to have worked on this ioctl on the kernel side. If I set up
> a GDB watchpoint on "filename", I can see it originally getting set, but
> not when it gets corrupted. I think this must be happening in the kernel.
> 
> I modified fw_set_port() as follows:
> 
> 657                     fd = open(filename, O_RDWR);
> 658                     if (fd < 0)
> 659                             continue;
> 660     
> 661                     memset(&get_info, 0, sizeof(get_info));
> 662                     memset(&reset, 0, sizeof(reset));
> 663                     get_info.version = IMPLEMENTED_CDEV_ABI_VERSION;
> 664                     get_info.rom = 0;
> 665                     get_info.rom_length = 0;
> 666                     get_info.bus_reset = ptr_to_u64(&reset);
> 667     asm("":::"memory");
> 668     fprintf(stderr,"HERE:%s\n", filename);
> 669     asm("":::"memory");
> 670                     if (ioctl(fd, FW_CDEV_IOC_GET_INFO, &get_info) < 0) {
> 671                             close(fd);
> 672                             continue;
> 673                     }
> 674     asm("":::"memory");
> 675     fprintf(stderr,"HERE2:%s\n", filename);
> 676     asm("":::"memory");
> 677     
> 678                     if (get_info.card != handle->ports[port].card) {
> 679                             close(fd);
> 680                             continue;
> 681                     }
> 
> strace shows:
> 
> open("/dev", O_RDONLY|O_NONBLOCK|O_LARGEFILE|O_DIRECTORY|O_CLOEXEC) = 7
> getdents(7, /* 709 entries */, 32768)   = 13880
> open("/dev/fw0", O_RDWR)                = 8
> write(2, "HERE:/dev/fw0\n", 14HERE:/dev/fw0
> )         = 14
> ioctl(8, FW_CDEV_IOC_GET_INFO, 0xffbb8114) = 0
> write(2, "HERE2:\377\377\377\377/fw0\n", 15HERE2:????????/fw0
> ) = 15
> epoll_ctl(3, EPOLL_CTL_ADD, 8, {EPOLLIN, {u32=167392044, u64=167392044}}) = 0
> 
> gdb again, from the start:
> 
> Breakpoint 2, fw_set_port (handle=handle@entry=0x804b018, port=port@entry=0) 
> at fw.c:668
> 668     fprintf(stderr,"HERE:%s\n", filename);
> (gdb) print filename
> $1 = "/dev/fw0\000:\372\367\274\000\001\000\000 
> \372\367T4\372\367\364\037\373\367\030\260\004\b"
> (gdb) print get_info
> $2 = {version = 4, rom_length = 0, rom = 0, bus_reset = 4294957064, 
> bus_reset_closure = 0, card = 0}
> (gdb) print reset
> $3 = {closure = 0, type = 0, node_id = 0, local_node_id = 0, bm_node_id = 0, 
> irm_node_id = 0, root_node_id = 0, generation = 0}
> (gdb) c
> Continuing.
> HERE:/dev/fw0
> 
> Breakpoint 3, fw_set_port (handle=handle@entry=0x804b018, port=port@entry=0) 
> at fw.c:675
> 675     fprintf(stderr,"HERE2:%s\n", filename);
> (gdb) print filename
> $4 = "\000\000\000\000/fw0\000:\372\367\274\000\001\000\000 
> \372\367T4\372\367\364\037\373\367\030\260\004\b"
> (gdb) print get_info
> $5 = {version = 5, rom_length = 88, rom = 0, bus_reset = 4294957064, 
> bus_reset_closure = 0, card = 0}
> (gdb) print reset
> $6 = {closure = 0, type = 0, node_id = 65472, local_node_id = 65472, 
> bm_node_id = 65472, irm_node_id = 65473, root_node_id = 65473, generation = 2}
> (gdb) print &filename
> $7 = (char (*)[32]) 0xffffd82c
> (gdb) print &get_info
> $8 = (struct fw_cdev_get_info *) 0xffffd7e4
> (gdb) print &reset   
> $9 = (struct fw_cdev_event_bus_reset *) 0xffffd808
> 
> Do you see what is wrong here? This is an amd64 kernel with i386 userland.
> 
> Cheers,
> 
> Simon-

Sounds like a 32-bits-userland-on-64-bits-kernel compatibility issue
indeed.  It is quite a long time ago that I runtime tested 32-on-64 ABI
compatibility; apparently we have a regression, or an old bug which only
now really hits.

For what it's worth, here is what I get on my amd64 PC...

        gcc (Gentoo 4.5.4 p1.0, pie-0.4.7) 4.5.4
        Target: x86_64-pc-linux-gnu
        sizeof(struct fw_cdev_event_bus_reset) == 40
        sizeof(struct fw_cdev_get_info) == 40

...and on my i686 PC:

        gcc (Gentoo 4.5.3-r2 p1.1, pie-0.4.7) 4.5.3
        Target: i686-pc-linux-gnu
        sizeof(struct fw_cdev_event_bus_reset) == 36
        sizeof(struct fw_cdev_get_info) == 36

In the kernel, drivers/firewire/core-cdev.c::ioctl_get_info(), we copy
with sizeof(struct fw_cdev_event_bus_reset)...

                ret = copy_to_user(u64_to_uptr(a->bus_reset),
                                   &bus_reset, sizeof(bus_reset));

...and in dispatch_ioctl() with the size that the user gave us:

        if (_IOC_DIR(cmd) & _IOC_READ)
                if (copy_to_user(arg, &buffer, _IOC_SIZE(cmd)))

In other words, we don't overrun the client's fw_cdev_get_info buffer
(because we let the client tell us how many bytes it thinks its buffer
has), but we obviously overrun the client's fw_cdev_event_bus_reset buffer.

This really looks to me as if we always did this wrong, didn't we?

Does anybody have a nifty idea how to get the compiler or preprocessor to
compute the size of struct fw_cdev_event_bus_reset as if it would be
__packed, IOW how to write
                ret = copy_to_user(u64_to_uptr(a->bus_reset),
                                   &bus_reset, 36);
in a better way?

PS,
from grepping through core-cdev.c, it looks like all our copy_from_user
are safe, and from all our copy_to_user only the one above with
sizeof(bus_reset) is unsafe.  We have one other copy_to_user where the
kernel sets the size; this cannot overrun the buffer but it could write the
results with a different alignment than the client expects.  This was
indeed the case with transaction response events before kernel 2.6.27.
-- 
Stefan Richter
-=====-===-- =-=- --=-=
http://arcgraph.de/sr/

----- End forwarded message -----


-- 
To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmas...@lists.debian.org

Reply via email to