Re: PATCH] firewire: add padding to some struct

From: JiSheng Zhang
Date: Sun Jul 20 2008 - 02:22:01 EST


On Sat, 19 Jul 2008 12:32:35 +0200
Stefan Richter <stefanr@xxxxxxxxxxxxxxxxx> wrote:

> JiSheng Zhang wrote:
> > On Fri, 18 Jul 2008 17:27:44 +0200
> > Mikael Pettersson <mikpe@xxxxxxxx> wrote:
> >> JiSheng Zhang writes:
> >> > --- old/drivers/firewire/fw-cdev.c 2008-07-14 05:51:29.000000000 +0800
> >> > +++ new/drivers/firewire/fw-cdev.c 2008-07-18 20:20:45.841328585 +0800
> >> > @@ -382,9 +382,9 @@
> >> >
> >> > response->response.type = FW_CDEV_EVENT_RESPONSE;
> >> > response->response.rcode = rcode;
> >> > - queue_event(client, &response->event,
> >> > - &response->response, sizeof(response->response),
> >> > - response->response.data, response->response.length);
> >> > + queue_event(client, &response->event, &response->response,
> >> > + sizeof(response->response) + response->response.length,
> >> > + NULL, 0);
> >> > }
> >>
> >> Neither of these look correct.
> >> If sizeof(struct ...) != offsetof(struct ..., data) as you claim is possible,
> >> then the old code will copy too much to/from ->response but the correct amount
> >> to/from ->response.data, and the new code will copy too much to/from ->response.data.
> >
> > The old code will copy 4 extra bytes totally on some platforms, the new code
> > is correct. The old one queue like this:
> > struct ...(excluding the padding bytes)|4 padding bytes|4 padding bytes|data
> >
> >> That's why C has offsetof():
> >>
> >> queue_event(client, &response->event,
> >> &response->response,
> >> offsetof(typeof(*response->responce), data), // I don't know the struct name
> >> response->response.data, response->response.length);
>
> sizeof(struct ...) != offsetof(struct ..., data) happens for example on
> x86-64.
>
> This is what I get with the current firewire drivers in a block read
> response from firecontrol on i686:
>
> Command: r . 0 0xfffff0000400 20
> reading from node 0, bus 1023, offset 0XFFFFF0000400 20 bytes
> read succeeded. Data follows (hex):
> 04 04 04 8F
> 31 33 39 34
> F0 00 A2 22
> 00 10 DC 56
> 00 FE D2 D4
> Ack code: complete
>
> And the same on x86-64:
>
> Command: r . 0 0xfffff0000400 20
> reading from node 0, bus 1023, offset 0XFFFFF0000400 20 bytes
> read succeeded. Data follows (hex):
> 04 04 04 8F
this is the 4 extra padding bytes
> 04 04 04 8F
> 31 33 39 34
> F0 00 A2 22
> 00 10 DC 56
here lost the last 4 bytes of data
> Ack code: complete
>
> Command: r . 0 0xfffff0000400 24
> reading from node 0, bus 1023, offset 0XFFFFF0000400 20 bytes
> read succeeded. Data follows (hex):
> 04 04 04 8F
> 04 04 04 8F
> 31 33 39 34
> F0 00 A2 22
> 00 10 DC 56
> 00 FE D2 D4
> Ack code: complete
>
> I used libraw1394 from Dan's git repo. Gscanbus shows exactly the same
> results. So, x86-64 and all other architectures where struct
> fw_cdev_event* are aligned on u64 boundaries are currently seriously
> broken... but nobody noticed it before. The only breakage which I saw
the read_topology_map in the testlibraw of libraw1394(with support to juju)
will show similar breakage.
> (and is obviously the result of this bug) is that gscanbus shows a wrong
> bus topology on x86-64 but the correct one on i686. The damage from
> this bug is limited though because
> - most applications send requests which get responses with 0 or 4
> bytes payload,
I think so.
> - no application (which can run on both old and new stack without
> change) uses address range mappings, i.e. get incoming requests.
>
> I'll look further into your proposed fix.
Thanks

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