| Message ID | 20170713074517.io64uezw3qf6oush@mwanda (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Jul 13, 2017 at 9:45 AM, Dan Carpenter <dan.carpenter@oracle.com> wrote: > There are hidden gotos in the ceph_decode_* macros. We need to set the > "err" variable on these error paths otherwise we end up returning > ERR_PTR(0) which is NULL. It causes NULL dereferences in the callers. > > Fixes: 278b1d709c6a ("libceph: ceph_decode_skip_* helpers") > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > diff --git a/net/ceph/osdmap.c b/net/ceph/osdmap.c > index 864789c5974e..c7521a847ef7 100644 > --- a/net/ceph/osdmap.c > +++ b/net/ceph/osdmap.c > @@ -510,6 +510,7 @@ static struct crush_map *crush_decode(void *pbyval, void *end) > } > } > > + err = -EINVAL; > ceph_decode_skip_map(p, end, 32, string, bad); /* type_map */ > ceph_decode_skip_map(p, end, 32, string, bad); /* name_map */ > ceph_decode_skip_map(p, end, 32, string, bad); /* rule_name_map */ > @@ -1825,9 +1826,9 @@ struct ceph_osdmap *osdmap_apply_incremental(void **p, void *end, > if (struct_v >= 3) { > /* new_erasure_code_profiles */ > ceph_decode_skip_map_of_map(p, end, string, string, string, > - bad); > + e_inval); > /* old_erasure_code_profiles */ > - ceph_decode_skip_set(p, end, string, bad); > + ceph_decode_skip_set(p, end, string, e_inval); > } > > if (struct_v >= 4) { Hi Dan, I applied osdmap_apply_incremental() hunk and fixed a similar bug in osdmap_decode() (it's a not NULL deref, that's why smatch didn't catch it). I posted a separate patch for crush_decode(). This is the second time in two months you are fixing it, so I wanted something more future-proof. See the end result at https://github.com/ceph/ceph-client/commits/testing. Thanks, Ilya -- To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jul 13, 2017 at 06:13:22PM +0200, Ilya Dryomov wrote: > > Hi Dan, > > I applied osdmap_apply_incremental() hunk and fixed a similar bug in > osdmap_decode() (it's a not NULL deref, that's why smatch didn't catch > it). > No... :/ Smatch complained about it, but I somehow marked it as done yesterday without sending a patch. Thanks for catching that. The Smatch check is just looking for places where we do ERR_PTR(0) or ERR_PTR(valid_ptr). That can be intentional, so there is some hand reviewing involved. regards, dan carpenter -- To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/ceph/osdmap.c b/net/ceph/osdmap.c index 864789c5974e..c7521a847ef7 100644 --- a/net/ceph/osdmap.c +++ b/net/ceph/osdmap.c @@ -510,6 +510,7 @@ static struct crush_map *crush_decode(void *pbyval, void *end) } } + err = -EINVAL; ceph_decode_skip_map(p, end, 32, string, bad); /* type_map */ ceph_decode_skip_map(p, end, 32, string, bad); /* name_map */ ceph_decode_skip_map(p, end, 32, string, bad); /* rule_name_map */ @@ -1825,9 +1826,9 @@ struct ceph_osdmap *osdmap_apply_incremental(void **p, void *end, if (struct_v >= 3) { /* new_erasure_code_profiles */ ceph_decode_skip_map_of_map(p, end, string, string, string, - bad); + e_inval); /* old_erasure_code_profiles */ - ceph_decode_skip_set(p, end, string, bad); + ceph_decode_skip_set(p, end, string, e_inval); } if (struct_v >= 4) {
There are hidden gotos in the ceph_decode_* macros. We need to set the "err" variable on these error paths otherwise we end up returning ERR_PTR(0) which is NULL. It causes NULL dereferences in the callers. Fixes: 278b1d709c6a ("libceph: ceph_decode_skip_* helpers") Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> -- To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html