Bug 8216
| Summary: | packet->header_size is not consistent with packet->header? | ||
|---|---|---|---|
| Product: | Drivers | Reporter: | Chongfeng Hu (loveminix) |
| Component: | IEEE1394 | Assignee: | Stefan Richter (stefanr) |
| Status: | CLOSED CODE_FIX | ||
| Severity: | high | ||
| Priority: | P2 | ||
| Hardware: | i386 | ||
| OS: | Linux | ||
| Kernel Version: | 2.6.20 | Subsystem: | |
| Regression: | --- | Bisected commit-id: | |
| Attachments: |
ieee1394: raw1394: prevent buffer overflow on RAW1394_REQ_ASYNC_SEND
ieee1394: remove usage of skb_queue as packet queue ieee1394: remove usage of skb_queue as packet queue |
||
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 |
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.