Project

General

Profile

Bug #2066

out of bounds write in dnode.c

Added by Martin Matuška about 8 years ago. Updated almost 8 years ago.

Status:
Rejected
Priority:
Normal
Assignee:
-
Category:
zfs - Zettabyte File System
Start date:
2012-02-02
Due date:
% Done:

0%

Estimated time:
Difficulty:
Bite-size
Tags:
needs-triage

Description

In usr/src/uts/common/fs/zfs/dnode.c, the OpenSolaris changeset 12684 resulted in the following code:

1068    idx = object & (epb-1);
1069
1070    ASSERT(DB_DNODE(db)->dn_type == DMU_OT_DNODE);
1071    children_dnodes = dmu_buf_get_user(&db->db);
1072    if (children_dnodes == NULL) {
1073        int i;
1074        dnode_children_t *winner;
1075        children_dnodes = kmem_alloc(sizeof (dnode_children_t) +
1076            (epb - 1) * sizeof (dnode_handle_t), KM_SLEEP);
1077        children_dnodes->dnc_count = epb;
1078        dnh = &children_dnodes->dnc_children[0];
1079        for (i = 0; i < epb; i++) {
1080            zrl_init(&dnh[i].dnh_zrlock);
1081            dnh[i].dnh_dnode = NULL;
1082        }
1083        if (winner = dmu_buf_set_user(&db->db, children_dnodes, NULL,
1084            dnode_buf_pageout)) {
1085            kmem_free(children_dnodes, sizeof (dnode_children_t) +
1086                (epb - 1) * sizeof (dnode_handle_t));
1087            children_dnodes = winner;
1088        }
1089    }
1090    ASSERT(children_dnodes->dnc_count == epb);

The kmem_alloc() statement on line 1075 allocates memory for (epb - 1) dnode_handle_t members.
The for loop in lines 1079-1082 iterates epb times (initializes members of struct dnode_handle_t).
So the last struct dnode_handle_t is initialized out of bounds.

We have discovered this in FreeBSD by kernel panics after our private change in line 1075 from kmem_alloc() to kmem_zalloc().

History

#2

Updated by Rich Lowe about 8 years ago

If you're wanting review of the webrev so we can get this integrated, it's more likely to be noticed if emailed to the developer@ list, than here (somewhat unfortunately, in my view, but there we go)

#3

Updated by Wenchun Cheng about 8 years ago

typedef struct dnode_children {
    size_t dnc_count;        /* number of children */
    dnode_handle_t dnc_children[1];    /* sized dynamically */
} dnode_children_t;

As dnode_childred_t already has 1 dnode_handle_t, hence the '-1'.. I guess if there is a kernel panic, should be something else?

#4

Updated by Martin Matuška about 8 years ago

I agree to Wenchun Cheng, please close this issue, we are going to investigate more deeply.

#5

Updated by Garrett D'Amore almost 8 years ago

  • Status changed from New to Rejected

Rejecting this as more investigation needed.

Also available in: Atom PDF