Project

General

Profile

Actions

Bug #2066

closed

out of bounds write in dnode.c

Added by Martin Matuška almost 10 years ago. Updated over 9 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
Gerrit CR:

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().

Actions #2

Updated by Rich Lowe almost 10 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)

Actions #3

Updated by Wenchun Cheng almost 10 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?

Actions #4

Updated by Martin Matuška almost 10 years ago

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

Actions #5

Updated by Garrett D'Amore over 9 years ago

  • Status changed from New to Rejected

Rejecting this as more investigation needed.

Actions

Also available in: Atom PDF