Bug 8216 - packet->header_size is not consistent with packet->header?
Summary: packet->header_size is not consistent with packet->header?
Alias: None
Product: Drivers
Classification: Unclassified
Component: IEEE1394 (show other bugs)
Hardware: i386 Linux
: P2 high
Assignee: Stefan Richter
Depends on:
Reported: 2007-03-15 23:52 UTC by Chongfeng Hu
Modified: 2007-05-02 07:11 UTC (History)
0 users

See Also:
Kernel Version: 2.6.20
Regression: ---
Bisected commit-id:

ieee1394: raw1394: prevent buffer overflow on RAW1394_REQ_ASYNC_SEND (1.10 KB, patch)
2007-03-16 06:25 UTC, Stefan Richter
Details | Diff
ieee1394: remove usage of skb_queue as packet queue (21.70 KB, patch)
2007-03-17 17:04 UTC, Stefan Richter
Details | Diff
ieee1394: remove usage of skb_queue as packet queue (21.87 KB, patch)
2007-03-26 17:12 UTC, Stefan Richter
Details | Diff

Description Chongfeng Hu 2007-03-15 23:52:51 UTC
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:

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.
Comment 1 Stefan Richter 2007-03-16 03:43:11 UTC
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
  - 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.
Comment 2 Stefan Richter 2007-03-16 06:25:35 UTC
Created attachment 10792 [details]
ieee1394: raw1394: prevent buffer overflow on RAW1394_REQ_ASYNC_SEND

fix for the raw1394 part of the problems
Comment 3 Stefan Richter 2007-03-17 17:04:04 UTC
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.
Comment 4 Stefan Richter 2007-03-18 03:57:00 UTC
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.
Comment 5 Stefan Richter 2007-03-26 17:12:17 UTC
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.
Comment 6 Stefan Richter 2007-05-02 07:11:07 UTC
cleanups of struct hpsb_packet's size fields merged for Linux 2.6.22-rc1

Note You need to log in before you can comment on or make changes to this bug.