Re: [PATCH v2] tftp.h: rework layout to work with fortification

["Carlos O'Donell" <carlos at systemhalted dot org>]

On Sun, May 6, 2012 at 1:14 AM, Mike Frysinger <vapier@gentoo.org> wrote:
> On Sunday 06 May 2012 00:12:39 Carlos O'Donell wrote:
>> On Thu, Apr 12, 2012 at 1:30 PM, Mike Frysinger wrote:
>> > --- a/inet/arpa/tftp.h
>> > +++ b/inet/arpa/tftp.h
>> > @@ -49,16 +49,17 @@
>> > ?struct tftphdr {
>> > ? ? ? ?short ? th_opcode;
>> > ? ? ? ?union {
>> > - ? ? ? ? ? ? ? unsigned short ?tu_block;
>> > - ? ? ? ? ? ? ? short ? tu_code;
>> > - ? ? ? ? ? ? ? char ? ?tu_stuff[1];
>> > - ? ? ? } __attribute__ ((__packed__)) th_u;
>> > - ? ? ? char ? ?th_data[1];
>> > + ? ? ? ? ? ? ? struct {
>> > + ? ? ? ? ? ? ? ? ? ? ? union {
>> > + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? unsigned short ?th_block;
>> > + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? short ? th_code;
>> > + ? ? ? ? ? ? ? ? ? ? ? } __attribute__ ((__packed__));
>> > + ? ? ? ? ? ? ? ? ? ? ? char th_data[0];
>> > + ? ? ? ? ? ? ? } __attribute__ ((__packed__));
>> > + ? ? ? ? ? ? ? char ? ?th_stuff[0];
>> > + ? ? ? } __attribute__ ((__packed__));
>> > ?} __attribute__ ((__packed__));
>>
>> I understand that fortification causes some problems here, but...
>>
>> Doesn't that break the ABI?
>
> how could it break ABI ? ?there are no funcs in glibc that take this struct as
> an argument or return it. ?i don't think there are any files that even include
> this header.
>
> as for random user code that includes this header, you don't malloc the struct
> or declare it on the stack because of the nature of it -- you overlay this on
> top of an arbitrarily long buffer that you received are are going to be
> sending. ?sizeof() is now 4 when it was 5, but that only affects things that
> want to access th_data, and that wouldn't make sense if you tried to
> instantiate the struct directly (e.g. struct tftphdr hdr;) since th_data,
> accordingly to the spec, has no defined length.

The ABI is comprised of two things:

* The layout.

* The size.

I verified the layout is the same (as you mentioned).

The change in sizeof() is a problem.

I went looking for example code that uses tftp.h.

Example 1:
~~~
...
int tftp_send_ack(struct tftphdr *tftp_hdr, int block)
{
	struct tftphdr *tftp_ack;
	struct sk_buff *skb;

	skb = alloc_skb(ETH_FRAME_LEN);
	udp_skb_reserve(skb);
	tftp_ack = (struct tftphdr *)skb_put(skb, sizeof(struct tftphdr));
	tftp_ack->th_opcode = htons(ACK);
	tftp_ack->th_block = htons(block);

	udp_send(skb, client_ip, TFTP, client_port);

	return 0;
}
...
~~~

Example 2:
~~~
...
char data_out[SEGSIZE+sizeof(struct tftphdr)];
char data_in[SEGSIZE+sizeof(struct tftphdr)];
...
static void
tftpd_write_file(struct tftphdr *hdr,
                 struct sockaddr_in *from_addr, int from_len)
{

    struct tftphdr *reply = (struct tftphdr *)data_out;
    struct tftphdr *response = (struct tftphdr *)data_in;
...
~~~

Both examples use sizeof(struct tfpthdr) which means that there could
conceivably be a situation like this:

* Application allocates N-1 bytes because it's using the new struct tfpthdr.
* Library copies N bytes because it's using the old struct tftphdr.
* Library crashes because the Nth byte is not a valid location.

You need to add padding to make the sizeof come out the same.

Does this example make sense?

> as for the offset's of the members, those are all the same.
>
>> > -#define ? ? ? ?th_block ? ? ? ?th_u.tu_block
>> > -#define ? ? ? ?th_code ? ? ? ? th_u.tu_code
>> > -#define ? ? ? ?th_stuff ? ? ? ?th_u.tu_stuff
>> > ?#define ? ? ? ?th_msg ? ? ? ? ?th_data
>>
>> Why do you remove the defines for th_block, th_code and th_stuff?
>> Isn't it possible that defines are used by userspace code?
>
> look at the struct -- i moved the names there

You used anonymous structures and unions which are not valid ISO C90
or C99 (though I *think* they are valid ISO C11).

The previous code worked with -Wall -Werror -pedantic, but the new code doesn't.

In summary:

* New code doesn't work with ISO C90 or C99.

* sizeof(struct tftp_hdr) should be the same before and after or

Can we work on fixing these issues?

I think the next steps are:

* Make it work in ISO C90/99 and -Wall -Werror -pedantic.

* Add padding so sizeof is the same.

Does that seem sensible?

Cheers,
Carlos.