Project

General

Profile

Actions

Bug #6825

closed

af-packet: possible free of unallocated memory

Added by Alexey Simakov 10 months ago. Updated 7 months ago.

Status:
Rejected
Priority:
Normal
Target version:
Affected Versions:
Effort:
low
Difficulty:
low
Label:

Description

AFPSetupRing function allocating memory for ptv->ring.v2/3 pointer.
In case of failing of allocating memory jump to postmap_err point happening,
where its try to free ptv->ring.v2/v3 field with check on nulls.
For ptr, which have should store allocated memory nothing happened, since
unsuccessful malloc call usually return NULL, but there is also try to
release memory for another field, which is uninitialized and in general
can contain any part of memory.

#ifdef HAVE_TPACKET_V3
    if (ptv->flags & AFP_TPACKET_V3) {
        ptv->ring.v3 = SCMalloc(ptv->req.v3.tp_block_nr * sizeof(*ptv->ring.v3)); <--- Trying to allocate memory for ptv->ring.v3
        if (!ptv->ring.v3) {
            SCLogError("%s: failed to alloc ring: %s", devname, strerror(errno));
            goto postmmap_err; <--- Allocating failed. Jump to postmmap_err label
        }
        for (i = 0; i < ptv->req.v3.tp_block_nr; ++i) {
            ptv->ring.v3[i].iov_base = ptv->ring_buf + (i * ptv->req.v3.tp_block_size);
            ptv->ring.v3[i].iov_len = ptv->req.v3.tp_block_size;
        }
    } else {
#endif
        /* allocate a ring for each frame header pointer*/
        ptv->ring.v2 = SCCalloc(ptv->req.v2.tp_frame_nr, sizeof(union thdr *));
        if (ptv->ring.v2 == NULL) {
            SCLogError("%s: failed to alloc ring: %s", devname, strerror(errno));
            goto postmmap_err;
        }
        /* fill the header ring with proper frame ptr*/
        ptv->frame_offset = 0;
        for (i = 0; i < ptv->req.v2.tp_block_nr; ++i) {
            void *base = &(ptv->ring_buf[i * ptv->req.v2.tp_block_size]);
            unsigned int j;
            for (j = 0; j < ptv->req.v2.tp_block_size / ptv->req.v2.tp_frame_size; ++j, ++ptv->frame_offset) {
                (((union thdr **)ptv->ring.v2)[ptv->frame_offset]) = base;
                base += ptv->req.v2.tp_frame_size;
            }
        }
        ptv->frame_offset = 0;
#ifdef HAVE_TPACKET_V3
    }
#endif

    return 0;

postmmap_err:
    munmap(ptv->ring_buf, ptv->ring_buflen);
    if (ptv->ring.v2) <--- Dangerous ptv->ring.v2 is uninitialized
        SCFree(ptv->ring.v2);
    if (ptv->ring.v3) <--- Its ok, since ptv->ring.v3 in NULL state after unsuccessful allocating
        SCFree(ptv->ring.v3);
mmap_err:
    /* Packet mmap does the cleaning when socket is closed */
    return AFP_FATAL_ERROR;
}

Probably, this is safe for most modern compilers since AFAIK they initialize all pointer to NULL by default,
but its not possible to rely on this behavior since its not standardized.

Actions #1

Updated by Victor Julien 8 months ago

  • Subject changed from source-af-packet possibel free of unallocated memory to af-packet: possible free of unallocated memory

v2 and v3 are in a union, so if one is set to NULL both are, right?

I think the code does look fishy though. If one would be non-NULL, both would be non-NULL and we'd have a double free.

    if (ptv->ring.v2)
        SCFree(ptv->ring.v2);
    if (ptv->ring.v3)
        SCFree(ptv->ring.v3);

But then again, it can only get here if v2 and v3 are both NULL due to the union. So ugly pattern, but harmless?

Would you agree?

Actions #2

Updated by Victor Julien 8 months ago

  • Description updated (diff)
Actions #3

Updated by Alexey Simakov 8 months ago

Yes, thats make sense, probably this is false positive detection in this case

Actions #4

Updated by Alexey Simakov 8 months ago

Seems, that I missed the fact in my analysis, that v2/v3 is union

Actions #5

Updated by Alexey Simakov 7 months ago

  • Status changed from New to Rejected
Actions

Also available in: Atom PDF