Bug #6825
closedaf-packet: possible free of unallocated memory
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.
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?
Updated by Alexey Simakov 8 months ago
Yes, thats make sense, probably this is false positive detection in this case
Updated by Alexey Simakov 8 months ago
Seems, that I missed the fact in my analysis, that v2/v3 is union