Distribution: kernel Problem Description: When I'm inspecting ieee1394 source code, I found that in struct hpsb_packet, field header is the header of the packet, and header_size is the size of the header. They should always keep consistent. However, I found one place where they are not updated consistently: drivers/ieee1394/ieee1394_core.c:149 struct hpsb_packet *hpsb_alloc_packet(size_t data_size) { struct hpsb_packet *packet = NULL; skb = alloc_skb(data_size + sizeof(*packet), GFP_ATOMIC); memset(skb->data, 0, data_size + sizeof(*packet)); packet = (struct hpsb_packet *)skb->data; packet->skb = skb; packet->header = packet->embedded_header; packet->state = hpsb_unused; packet->generation = -1; } in the piece of code, skb->data is first memset to zero, and then packet points to it, so now all the fields in packet are zero. Then packet->header got a valid value, but packet->header_size isn't updated together. This might cause serious consistency problems. Steps to reproduce: I found this suspicious spot with the help of a code-checking tool.
The problem you point out isn't one in my opinion. In particular, hpsb_alloc_packet does have only limited knowledge about the header size: It can be aware of the maximum size of the default header storage (packet->embedded_header). But it is unaware of the _actual_ size of the header because that one depends on the type of packet and is filled in later. Anyway. What's important is is who calls hpsb_alloc_packet and what is then done to the new packet. There is 1 call in ieee1394_core.c, 7 in ieee1394_transactions.c, 1 in eth1394.c. These are all followed by fill_ functions which set the correct header_size according to the type of packet. And then there is 1 call in raw1394.c which allows userspace apps to generate arbitrary packets. And here seem to be two _real_ bugs: - We copy user data into packet->header without checking if we have enough storage for the header contents. - We set packet->header_size to the user-supplied value without checking for sanity. But I see more issues: - We apparently never use any other storage for packet->header than packet->embedded_header. We just should make header itself to an array. - There is not only no code which allocates a packet->header different from packet->embedded_header, there is also no code which would deallocate such storage. Besides, the embedding of hpsb_packet with sk_buff imposes an unnecessary overhead on size and handling of packets.
Created attachment 10792 [details] ieee1394: raw1394: prevent buffer overflow on RAW1394_REQ_ASYNC_SEND fix for the raw1394 part of the problems
Created attachment 10809 [details] ieee1394: remove usage of skb_queue as packet queue This patch clarifies the distinction of allocated size versus filled-in size for hpsb_packet.data. hpsb_packet.header is now definitely a fixed-size allocation, therefore only its filled-in size is tracked. The bulk of the patch deals with the removal of hpsb_packet.skb.
The changes to ieee1394_core.h::struct hpsb_packet and ieee1394_core.c::hpsb_alloc_packet() should clarify (to human readers, perhaps not to code checking tools) what the role of packet->{data,header}_size is and who sets them how.
Created attachment 10954 [details] ieee1394: remove usage of skb_queue as packet queue There was actually no buffer overflow in raw1394 due to an implementation detail of hpsb_alloc_packet. The updated patch makes raw1394 independent of this detail.
cleanups of struct hpsb_packet's size fields merged for Linux 2.6.22-rc1