by Chris Wright

[permalink] [raw]

Subject: Re: [PATCH 00/20] ceph: Ceph distributed file system client v0.10

* Sage Weil ([email protected]) wrote:
> I don't always verify that things like element counts are "sane", though,
> so there's currently the possibility of trying to allocate large chunks of
> memory.

There's also the potential of allocating a small amount of memory w/ a
large element count if you overflow on multiply. You should definitely
protect against this if you have such code in ceph client (kcalloc has
simple example defense).

thanks,
-chris

2009-07-18 04:40:01

by Sage Weil

[permalink] [raw]

Subject: Re: [PATCH 00/20] ceph: Ceph distributed file system client v0.10

On Fri, 17 Jul 2009, Chris Wright wrote:
> * Sage Weil ([email protected]) wrote:
> > I don't always verify that things like element counts are "sane", though,
> > so there's currently the possibility of trying to allocate large chunks of
> > memory.
>
> There's also the potential of allocating a small amount of memory w/ a
> large element count if you overflow on multiply. You should definitely
> protect against this if you have such code in ceph client (kcalloc has
> simple example defense).

Good call. I've audited all the k[mz]alloc call sites and switched to
kcalloc for all the decode num * size allocations, or added a similar
check for overflow.

Thanks-
sage

---
diff --git a/src/include/ceph_fs.h b/src/include/ceph_fs.h
index c7ca9ba..6242cf2 100644
--- a/src/include/ceph_fs.h
+++ b/src/include/ceph_fs.h
@@ -31,6 +31,9 @@

#define CEPH_INO_ROOT 1

+/* arbitrary limit on max # of monitors (cluster of 3 is typical) */
+#define CEPH_MAX_MON 31
+

/*
* "Frags" are a way to describe a subset of a 32-bit number space,
diff --git a/src/kernel/inode.c b/src/kernel/inode.c
index c22e570..6758f69 100644
--- a/src/kernel/inode.c
+++ b/src/kernel/inode.c
@@ -1926,7 +1926,8 @@ start:
xattr_version = ci->i_xattrs.version;
spin_unlock(&inode->i_lock);

- xattrs = kmalloc(numattr*sizeof(struct ceph_xattr *), GFP_NOFS);
+ xattrs = kcalloc(numattr, sizeof(struct ceph_xattr *),
+ GFP_NOFS);
err = -ENOMEM;
if (!xattrs)
goto bad_lock;
@@ -2177,7 +2178,7 @@ static int ceph_sync_setxattr(struct dentry *dentry, const char *name,
/* copy value into some pages */
nr_pages = calc_pages_for(0, size);
if (nr_pages) {
- pages = kmalloc(sizeof(pages)*nr_pages, GFP_NOFS);
+ pages = kmalloc(sizeof(pages[0])*nr_pages, GFP_NOFS);
if (!pages)
return -ENOMEM;
err = -ENOMEM;
diff --git a/src/kernel/mds_client.c b/src/kernel/mds_client.c
index 6237134..dd5630b 100644
--- a/src/kernel/mds_client.c
+++ b/src/kernel/mds_client.c
@@ -129,10 +129,10 @@ static int parse_reply_info_dir(void **p, void *end,

/* alloc large array */
info->dir_nr = num;
- info->dir_in = kmalloc(num * (sizeof(*info->dir_in) +
- sizeof(*info->dir_dname) +
- sizeof(*info->dir_dname_len) +
- sizeof(*info->dir_dlease)),
+ info->dir_in = kcalloc(num, sizeof(*info->dir_in) +
+ sizeof(*info->dir_dname) +
+ sizeof(*info->dir_dname_len) +
+ sizeof(*info->dir_dlease),
GFP_NOFS);
if (info->dir_in == NULL) {
err = -ENOMEM;
@@ -312,7 +312,7 @@ static struct ceph_mds_session *register_session(struct ceph_mds_client *mdsc,
struct ceph_mds_session **sa;

dout("register_session realloc to %d\n", newmax);
- sa = kzalloc(newmax * sizeof(void *), GFP_NOFS);
+ sa = kcalloc(newmax, sizeof(void *), GFP_NOFS);
if (sa == NULL)
return ERR_PTR(-ENOMEM);
if (mdsc->sessions) {
diff --git a/src/kernel/mdsmap.c b/src/kernel/mdsmap.c
index b545850..e1a86ae 100644
--- a/src/kernel/mdsmap.c
+++ b/src/kernel/mdsmap.c
@@ -65,8 +65,8 @@ struct ceph_mdsmap *ceph_mdsmap_decode(void **p, void *end)
ceph_decode_64(p, m->m_max_file_size);
ceph_decode_32(p, m->m_max_mds);

- m->m_addr = kzalloc(m->m_max_mds*sizeof(*m->m_addr), GFP_NOFS);
- m->m_state = kzalloc(m->m_max_mds*sizeof(*m->m_state), GFP_NOFS);
+ m->m_addr = kcalloc(m->m_max_mds, sizeof(*m->m_addr), GFP_NOFS);
+ m->m_state = kcalloc(m->m_max_mds, sizeof(*m->m_state), GFP_NOFS);
if (m->m_addr == NULL || m->m_state == NULL)
goto badmem;

@@ -102,7 +102,7 @@ struct ceph_mdsmap *ceph_mdsmap_decode(void **p, void *end)
/* pg_pools */
ceph_decode_32_safe(p, end, n, bad);
m->m_num_data_pg_pools = n;
- m->m_data_pg_pools = kmalloc(sizeof(u32)*n, GFP_NOFS);
+ m->m_data_pg_pools = kcalloc(n, sizeof(u32), GFP_NOFS);
if (!m->m_data_pg_pools)
goto badmem;
ceph_decode_need(p, end, sizeof(u32)*(n+1), bad);
diff --git a/src/kernel/messenger.c b/src/kernel/messenger.c
index a5828ca..845faf2 100644
--- a/src/kernel/messenger.c
+++ b/src/kernel/messenger.c
@@ -303,7 +303,7 @@ static struct ceph_connection *new_connection(struct ceph_messenger *msgr)
{
struct ceph_connection *con;

- con = kzalloc(sizeof(struct ceph_connection), GFP_NOFS);
+ con = kzalloc(sizeof(*con), GFP_NOFS);
if (con == NULL)
return NULL;
con->msgr = msgr;
diff --git a/src/kernel/mon_client.c b/src/kernel/mon_client.c
index abee6dc..23ce141 100644
--- a/src/kernel/mon_client.c
+++ b/src/kernel/mon_client.c
@@ -33,6 +33,8 @@ struct ceph_monmap *ceph_monmap_decode(void *p, void *end)
ceph_decode_need(&p, end, num_mon*sizeof(m->mon_inst[0]), bad);

/* The encoded and decoded sizes match. */
+ if (num_mon >= CEPH_MAX_MON)
+ goto bad;
m = kmalloc(sizeof(*m) + sizeof(m->mon_inst[0])*num_mon, GFP_NOFS);
if (m == NULL)
return ERR_PTR(-ENOMEM);
diff --git a/src/kernel/osdmap.c b/src/kernel/osdmap.c
index 23bef3b..fef1574 100644
--- a/src/kernel/osdmap.c
+++ b/src/kernel/osdmap.c
@@ -78,10 +78,10 @@ static int crush_decode_list_bucket(void **p, void *end,
{
int j;
dout("crush_decode_list_bucket %p to %p\n", *p, end);
- b->item_weights = kmalloc(b->h.size * sizeof(u32), GFP_NOFS);
+ b->item_weights = kcalloc(b->h.size, sizeof(u32), GFP_NOFS);
if (b->item_weights == NULL)
return -ENOMEM;
- b->sum_weights = kmalloc(b->h.size * sizeof(u32), GFP_NOFS);
+ b->sum_weights = kcalloc(b->h.size, sizeof(u32), GFP_NOFS);
if (b->sum_weights == NULL)
return -ENOMEM;
ceph_decode_need(p, end, 2 * b->h.size * sizeof(u32), bad);
@@ -100,7 +100,7 @@ static int crush_decode_tree_bucket(void **p, void *end,
int j;
dout("crush_decode_tree_bucket %p to %p\n", *p, end);
ceph_decode_32_safe(p, end, b->num_nodes, bad);
- b->node_weights = kmalloc(b->num_nodes * sizeof(u32), GFP_NOFS);
+ b->node_weights = kcalloc(b->num_nodes, sizeof(u32), GFP_NOFS);
if (b->node_weights == NULL)
return -ENOMEM;
ceph_decode_need(p, end, b->num_nodes * sizeof(u32), bad);
@@ -116,10 +116,10 @@ static int crush_decode_straw_bucket(void **p, void *end,
{
int j;
dout("crush_decode_straw_bucket %p to %p\n", *p, end);
- b->item_weights = kmalloc(b->h.size * sizeof(u32), GFP_NOFS);
+ b->item_weights = kcalloc(b->h.size, sizeof(u32), GFP_NOFS);
if (b->item_weights == NULL)
return -ENOMEM;
- b->straws = kmalloc(b->h.size * sizeof(u32), GFP_NOFS);
+ b->straws = kcalloc(b->h.size, sizeof(u32), GFP_NOFS);
if (b->straws == NULL)
return -ENOMEM;
ceph_decode_need(p, end, 2 * b->h.size * sizeof(u32), bad);
@@ -158,17 +158,17 @@ static struct crush_map *crush_decode(void *pbyval, void *end)
ceph_decode_32(p, c->max_rules);
ceph_decode_32(p, c->max_devices);

- c->device_parents = kmalloc(c->max_devices * sizeof(u32), GFP_NOFS);
+ c->device_parents = kcalloc(c->max_devices, sizeof(u32), GFP_NOFS);
if (c->device_parents == NULL)
goto badmem;
- c->bucket_parents = kmalloc(c->max_buckets * sizeof(u32), GFP_NOFS);
+ c->bucket_parents = kcalloc(c->max_buckets, sizeof(u32), GFP_NOFS);
if (c->bucket_parents == NULL)
goto badmem;

- c->buckets = kmalloc(c->max_buckets * sizeof(*c->buckets), GFP_NOFS);
+ c->buckets = kcalloc(c->max_buckets, sizeof(*c->buckets), GFP_NOFS);
if (c->buckets == NULL)
goto badmem;
- c->rules = kmalloc(c->max_rules * sizeof(*c->rules), GFP_NOFS);
+ c->rules = kcalloc(c->max_rules, sizeof(*c->rules), GFP_NOFS);
if (c->rules == NULL)
goto badmem;

@@ -217,10 +217,10 @@ static struct crush_map *crush_decode(void *pbyval, void *end)
dout("crush_decode bucket size %d off %x %p to %p\n",
b->size, (int)(*p-start), *p, end);

- b->items = kmalloc(b->size * sizeof(__s32), GFP_NOFS);
+ b->items = kcalloc(b->size, sizeof(__s32), GFP_NOFS);
if (b->items == NULL)
goto badmem;
- b->perm = kmalloc(b->size * sizeof(u32), GFP_NOFS);
+ b->perm = kcalloc(b->size, sizeof(u32), GFP_NOFS);
if (b->perm == NULL)
goto badmem;
b->perm_n = 0;
@@ -276,7 +276,10 @@ static struct crush_map *crush_decode(void *pbyval, void *end)

/* len */
ceph_decode_32_safe(p, end, yes, bad);
-
+#if BITS_PER_LONG == 32
+ if (yes > ULONG_MAX / sizeof(struct crush_rule_step))
+ goto bad;
+#endif
r = c->rules[i] = kmalloc(sizeof(*r) +
yes*sizeof(struct crush_rule_step),
GFP_NOFS);
@@ -331,9 +334,9 @@ static int osdmap_set_max_osd(struct ceph_osdmap *map, int max)
struct ceph_entity_addr *addr;
u32 *weight;

- state = kzalloc(max * sizeof(*state), GFP_NOFS);
- addr = kzalloc(max * sizeof(*addr), GFP_NOFS);
- weight = kzalloc(max * sizeof(*weight), GFP_NOFS);
+ state = kcalloc(max, sizeof(*state), GFP_NOFS);
+ addr = kcalloc(max, sizeof(*addr), GFP_NOFS);
+ weight = kcalloc(max, sizeof(*weight), GFP_NOFS);
if (state == NULL || addr == NULL || weight == NULL) {
kfree(state);
kfree(addr);
@@ -381,7 +384,7 @@ struct ceph_osdmap *osdmap_decode(void **p, void *end)
ceph_decode_copy(p, &map->modified, sizeof(map->modified));

ceph_decode_32(p, map->num_pools);
- map->pg_pool = kmalloc(map->num_pools * sizeof(*map->pg_pool),
+ map->pg_pool = kcalloc(map->num_pools, sizeof(*map->pg_pool),
GFP_NOFS);
if (!map->pg_pool) {
err = -ENOMEM;
@@ -523,8 +526,9 @@ struct ceph_osdmap *apply_incremental(void **p, void *end,
while (len--) {
ceph_decode_32_safe(p, end, pool, bad);
if (pool >= map->num_pools) {
- void *pg_pool = kzalloc((pool+1)*sizeof(*map->pg_pool),
- GFP_NOFS);
+ void *pg_pool = kcalloc(pool + 1,
+ sizeof(*map->pg_pool),
+ GFP_NOFS);
if (!pg_pool) {
err = -ENOMEM;
goto bad;
diff --git a/src/kernel/snap.c b/src/kernel/snap.c
index f3e0685..e07d776 100644
--- a/src/kernel/snap.c
+++ b/src/kernel/snap.c
@@ -299,6 +299,8 @@ static int build_snap_context(struct ceph_snap_realm *realm)

/* alloc new snap context */
err = -ENOMEM;
+ if (num > ULONG_MAX / sizeof(u64) - sizeof(*snapc))
+ goto fail;
snapc = kzalloc(sizeof(*snapc) + num*sizeof(u64), GFP_NOFS);
if (!snapc)
goto fail;
@@ -374,7 +376,7 @@ static int dup_array(u64 **dst, __le64 *src, int num)

kfree(*dst);
if (num) {
- *dst = kmalloc(sizeof(u64) * num, GFP_NOFS);
+ *dst = kcalloc(num, sizeof(u64), GFP_NOFS);
if (!*dst)
return -ENOMEM;
for (i = 0; i < num; i++)