[PATCH 0/3] ceph: fix multiple out-of-bounds checks

[PATCH 0/3] ceph: fix multiple out-of-bounds checks

[Xi Wang]

Fix several possible out-of-bounds pointers if the incoming message is
corrupted or malicious.

Xi Wang (3):
  ceph: fix out-of-bounds pointers in parse_reply_info()
  ceph: fix bounds check macros ceph_decode_need and ceph_encode_need
  ceph: avoid panic with mismatched symbolic link sizes in fill_inode()

 fs/ceph/inode.c             |   10 +++++-----
 fs/ceph/mds_client.c        |    2 ++
 include/linux/ceph/decode.h |    9 +++++++--
 3 files changed, 14 insertions(+), 7 deletions(-)

-- 
1.7.5.4

[PATCH 1/3] ceph: fix out-of-bounds pointers in parse_reply_info()

[Xi Wang]

Given a large len, the pointer p+len used in further parsing could be
out of bounds.

Signed-off-by: Xi Wang <xi.wang@gmail.com>
---
 fs/ceph/mds_client.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
index 6203d80..be1415f 100644
--- a/fs/ceph/mds_client.c
+++ b/fs/ceph/mds_client.c
@@ -262,6 +262,7 @@ static int parse_reply_info(struct ceph_msg *msg,
 	/* trace */
 	ceph_decode_32_safe(&p, end, len, bad);
 	if (len > 0) {
+		ceph_decode_need(&p, end, len, bad);
 		err = parse_reply_info_trace(&p, p+len, info, features);
 		if (err < 0)
 			goto out_bad;
@@ -270,6 +271,7 @@ static int parse_reply_info(struct ceph_msg *msg,
 	/* extra */
 	ceph_decode_32_safe(&p, end, len, bad);
 	if (len > 0) {
+		ceph_decode_need(&p, end, len, bad);
 		err = parse_reply_info_extra(&p, p+len, info, features);
 		if (err < 0)
 			goto out_bad;
-- 
1.7.5.4

[PATCH 2/3] ceph: fix bounds check macros ceph_decode_need and ceph_encode_need

[Xi Wang]

Given a large n, the bounds check (*p + n > end) can be bypassed due to
pointer wraparound.  A safer check is (n > end - *p).

Signed-off-by: Xi Wang <xi.wang@gmail.com>
---
 include/linux/ceph/decode.h |    9 +++++++--
 1 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/include/linux/ceph/decode.h b/include/linux/ceph/decode.h
index c5b6939..ea6db7b 100644
--- a/include/linux/ceph/decode.h
+++ b/include/linux/ceph/decode.h
@@ -12,6 +12,11 @@
  *   void *end    pointer to end of buffer (last byte + 1)
  */
 
+static inline int ceph_need(void **p, void *end, size_t n)
+{
+	return ((end < *p) || (n > end - *p));
+}
+
 static inline u64 ceph_decode_64(void **p)
 {
 	u64 v = get_unaligned_le64(*p);
@@ -47,7 +52,7 @@ static inline void ceph_decode_copy(void **p, void *pv, size_t n)
  */
 #define ceph_decode_need(p, end, n, bad)		\
 	do {						\
-		if (unlikely(*(p) + (n) > (end))) 	\
+		if (unlikely(ceph_need(p, end, n)))	\
 			goto bad;			\
 	} while (0)
 
@@ -166,7 +171,7 @@ static inline void ceph_encode_string(void **p, void *end,
 
 #define ceph_encode_need(p, end, n, bad)		\
 	do {						\
-		if (unlikely(*(p) + (n) > (end))) 	\
+		if (unlikely(ceph_need(p, end, n)))	\
 			goto bad;			\
 	} while (0)
 
-- 
1.7.5.4

[PATCH 3/3] ceph: avoid panic with mismatched symbolic link sizes in fill_inode()

[Xi Wang]

Return -EINVAL with mismatched iinfo->symlink_len and inode->i_size.

Also use kstrndup rather than kmalloc/memcpy.

Signed-off-by: Xi Wang <xi.wang@gmail.com>
---
 fs/ceph/inode.c |   10 +++++-----
 1 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
index 87fb132..d100cd6 100644
--- a/fs/ceph/inode.c
+++ b/fs/ceph/inode.c
@@ -678,18 +678,18 @@ static int fill_inode(struct inode *inode,
 	case S_IFLNK:
 		inode->i_op = &ceph_symlink_iops;
 		if (!ci->i_symlink) {
-			int symlen = iinfo->symlink_len;
+			u32 symlen = iinfo->symlink_len;
 			char *sym;
 
-			BUG_ON(symlen != inode->i_size);
 			spin_unlock(&ci->i_ceph_lock);
+			err = -EINVAL;
+			if (symlen != inode->i_size)
+				goto out;
 
 			err = -ENOMEM;
-			sym = kmalloc(symlen+1, GFP_NOFS);
+			sym = kstrndup(iinfo->symlink, symlen, GFP_NOFS);
 			if (!sym)
 				goto out;
-			memcpy(sym, iinfo->symlink, symlen);
-			sym[symlen] = 0;
 
 			spin_lock(&ci->i_ceph_lock);
 			if (!ci->i_symlink)
-- 
1.7.5.4

Re: [PATCH 2/3] ceph: fix bounds check macros ceph_decode_need and ceph_encode_need

[Alex Elder]

On 12/14/2011 02:24 PM, Xi Wang wrote:
> Given a large n, the bounds check (*p + n>  end) can be bypassed due to
> pointer wraparound.  A safer check is (n>  end - *p).
>
> Signed-off-by: Xi Wang<xi.wang@gmail.com>

I noticed this proposed change never got committed.

It looks good, but I don't like the name "ceph_need()".

I am planning to pull this in soon, modified like this:

static inline int ceph_need_ok(void **p, void *end, size_t n)
{
        return end >= *p && n <= end - *p;
}

And then used like this:

                if (!likely(ceph_need_ok(p, end, n)))

If you have an objection to that, please say so soon
(and if you have no objection, please ACK).

Reviewed-by: Alex Elder <elder@dreamhost.com>

> ---
>   include/linux/ceph/decode.h |    9 +++++++--
>   1 files changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/ceph/decode.h b/include/linux/ceph/decode.h
> index c5b6939..ea6db7b 100644
> --- a/include/linux/ceph/decode.h
> +++ b/include/linux/ceph/decode.h
> @@ -12,6 +12,11 @@
>    *   void *end    pointer to end of buffer (last byte + 1)
>    */
>
> +static inline int ceph_need(void **p, void *end, size_t n)
> +{
> +	return ((end<  *p) || (n>  end - *p));
> +}
> +
>   static inline u64 ceph_decode_64(void **p)
>   {
>   	u64 v = get_unaligned_le64(*p);
> @@ -47,7 +52,7 @@ static inline void ceph_decode_copy(void **p, void *pv, size_t n)
>    */
>   #define ceph_decode_need(p, end, n, bad)		\
>   	do {						\
> -		if (unlikely(*(p) + (n)>  (end))) 	\
> +		if (unlikely(ceph_need(p, end, n)))	\
>   			goto bad;			\
>   	} while (0)
>
> @@ -166,7 +171,7 @@ static inline void ceph_encode_string(void **p, void *end,
>
>   #define ceph_encode_need(p, end, n, bad)		\
>   	do {						\
> -		if (unlikely(*(p) + (n)>  (end))) 	\
> +		if (unlikely(ceph_need(p, end, n)))	\
>   			goto bad;			\
>   	} while (0)
>

Re: [PATCH 2/3] ceph: fix bounds check macros ceph_decode_need and ceph_encode_need

[Sage Weil]

On Wed, 18 Apr 2012, Alex Elder wrote:
> On 12/14/2011 02:24 PM, Xi Wang wrote:
> > Given a large n, the bounds check (*p + n>  end) can be bypassed due to
> > pointer wraparound.  A safer check is (n>  end - *p).
> > 
> > Signed-off-by: Xi Wang<xi.wang@gmail.com>
> 
> I noticed this proposed change never got committed.
> 
> It looks good, but I don't like the name "ceph_need()".
> 
> I am planning to pull this in soon, modified like this:
> 
> static inline int ceph_need_ok(void **p, void *end, size_t n)

Would something like ceph_has_room() make more sense?

> {
>        return end >= *p && n <= end - *p;
> }
> 
> And then used like this:
> 
>                if (!likely(ceph_need_ok(p, end, n)))
> 
> If you have an objection to that, please say so soon
> (and if you have no objection, please ACK).
> 
> Reviewed-by: Alex Elder <elder@dreamhost.com>
> 
> > ---
> >   include/linux/ceph/decode.h |    9 +++++++--
> >   1 files changed, 7 insertions(+), 2 deletions(-)
> > 
> > diff --git a/include/linux/ceph/decode.h b/include/linux/ceph/decode.h
> > index c5b6939..ea6db7b 100644
> > --- a/include/linux/ceph/decode.h
> > +++ b/include/linux/ceph/decode.h
> > @@ -12,6 +12,11 @@
> >    *   void *end    pointer to end of buffer (last byte + 1)
> >    */
> > 
> > +static inline int ceph_need(void **p, void *end, size_t n)
> > +{
> > +	return ((end<  *p) || (n>  end - *p));
> > +}
> > +
> >   static inline u64 ceph_decode_64(void **p)
> >   {
> >   	u64 v = get_unaligned_le64(*p);
> > @@ -47,7 +52,7 @@ static inline void ceph_decode_copy(void **p, void *pv,
> > size_t n)
> >    */
> >   #define ceph_decode_need(p, end, n, bad)		\
> >   	do {						\
> > -		if (unlikely(*(p) + (n)>  (end))) 	\
> > +		if (unlikely(ceph_need(p, end, n)))	\
> >   			goto bad;			\
> >   	} while (0)
> > 
> > @@ -166,7 +171,7 @@ static inline void ceph_encode_string(void **p, void
> > *end,
> > 
> >   #define ceph_encode_need(p, end, n, bad)		\
> >   	do {						\
> > -		if (unlikely(*(p) + (n)>  (end))) 	\
> > +		if (unlikely(ceph_need(p, end, n)))	\
> >   			goto bad;			\
> >   	} while (0)
> > 
> 
> --
> 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
> 
> 

Re: [PATCH 2/3] ceph: fix bounds check macros ceph_decode_need and ceph_encode_need

[Alex Elder]

On 04/18/2012 12:53 PM, Sage Weil wrote:
> Would something like ceph_has_room() make more sense?

Yes, and standing by itself it reads a lot better too.

The "need_ok" came from the fact that it was only ever
called by ceph_need_*() macros.

Without objection I'll change it to ceph_has_room()
before I commit it for testing.

					-Alex