| Message ID | 20230712120718.28904-1-idryomov@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | libceph: harden msgr2.1 frame segment length checks | expand |
On 7/12/23 7:07 AM, Ilya Dryomov wrote: > ceph_frame_desc::fd_lens is an int array. decode_preamble() thus > effectively casts u32 -> int but the checks for segment lengths are > written as if on unsigned values. While reading in HELLO or one of the > AUTH frames (before authentication is completed), arithmetic in > head_onwire_len() can get duped by negative ctrl_len and produce > head_len which is less than CEPH_PREAMBLE_LEN but still positive. > This would lead to a buffer overrun in prepare_read_control() as the > preamble gets copied to the newly allocated buffer of size head_len. > > Cc: stable@vger.kernel.org > Fixes: cd1a677cad99 ("libceph, ceph: implement msgr2.1 protocol (crc and secure modes)") > Reported-by: Thelford Williams <thelford@google.com> > Signed-off-by: Ilya Dryomov <idryomov@gmail.com> > --- > net/ceph/messenger_v2.c | 41 ++++++++++++++++++++++++++--------------- > 1 file changed, 26 insertions(+), 15 deletions(-) > > diff --git a/net/ceph/messenger_v2.c b/net/ceph/messenger_v2.c > index 1a888b86a494..1df1d29dee92 100644 > --- a/net/ceph/messenger_v2.c > +++ b/net/ceph/messenger_v2.c > @@ -390,6 +390,8 @@ static int head_onwire_len(int ctrl_len, bool secure) > int head_len; > int rem_len; > > + BUG_ON(ctrl_len < 0 || ctrl_len > CEPH_MSG_MAX_CONTROL_LEN); Doesn't the ctrl_len ultimately come from the outside? If so you should not do BUG_ON() with bad values. Could you have this function return 0 for a bad value instead? It means you need to handle it in the callers, but better than than killing the machine. -Alex > + > if (secure) { > head_len = CEPH_PREAMBLE_SECURE_LEN; > if (ctrl_len > CEPH_PREAMBLE_INLINE_LEN) { > @@ -408,6 +410,10 @@ static int head_onwire_len(int ctrl_len, bool secure) > static int __tail_onwire_len(int front_len, int middle_len, int data_len, > bool secure) > { > + BUG_ON(front_len < 0 || front_len > CEPH_MSG_MAX_FRONT_LEN || > + middle_len < 0 || middle_len > CEPH_MSG_MAX_MIDDLE_LEN || > + data_len < 0 || data_len > CEPH_MSG_MAX_DATA_LEN); > + > if (!front_len && !middle_len && !data_len) > return 0; > > @@ -520,29 +526,34 @@ static int decode_preamble(void *p, struct ceph_frame_desc *desc) > desc->fd_aligns[i] = ceph_decode_16(&p); > } > > - /* > - * This would fire for FRAME_TAG_WAIT (it has one empty > - * segment), but we should never get it as client. > - */ > - if (!desc->fd_lens[desc->fd_seg_cnt - 1]) { > - pr_err("last segment empty\n"); > + if (desc->fd_lens[0] < 0 || > + desc->fd_lens[0] > CEPH_MSG_MAX_CONTROL_LEN) { > + pr_err("bad control segment length %d\n", desc->fd_lens[0]); > return -EINVAL; > } > - > - if (desc->fd_lens[0] > CEPH_MSG_MAX_CONTROL_LEN) { > - pr_err("control segment too big %d\n", desc->fd_lens[0]); > + if (desc->fd_lens[1] < 0 || > + desc->fd_lens[1] > CEPH_MSG_MAX_FRONT_LEN) { > + pr_err("bad front segment length %d\n", desc->fd_lens[1]); > return -EINVAL; > } > - if (desc->fd_lens[1] > CEPH_MSG_MAX_FRONT_LEN) { > - pr_err("front segment too big %d\n", desc->fd_lens[1]); > + if (desc->fd_lens[2] < 0 || > + desc->fd_lens[2] > CEPH_MSG_MAX_MIDDLE_LEN) { > + pr_err("bad middle segment length %d\n", desc->fd_lens[2]); > return -EINVAL; > } > - if (desc->fd_lens[2] > CEPH_MSG_MAX_MIDDLE_LEN) { > - pr_err("middle segment too big %d\n", desc->fd_lens[2]); > + if (desc->fd_lens[3] < 0 || > + desc->fd_lens[3] > CEPH_MSG_MAX_DATA_LEN) { > + pr_err("bad data segment length %d\n", desc->fd_lens[3]); > return -EINVAL; > } > - if (desc->fd_lens[3] > CEPH_MSG_MAX_DATA_LEN) { > - pr_err("data segment too big %d\n", desc->fd_lens[3]); > + > + /* > + * This would fire for FRAME_TAG_WAIT (it has one empty > + * segment), but we should never get it as client. > + */ > + if (!desc->fd_lens[desc->fd_seg_cnt - 1]) { > + pr_err("last segment empty, segment count %d\n", > + desc->fd_seg_cnt); > return -EINVAL; > } >
On Wed, Jul 12, 2023 at 2:34 PM Alex Elder <elder@ieee.org> wrote: > > On 7/12/23 7:07 AM, Ilya Dryomov wrote: > > ceph_frame_desc::fd_lens is an int array. decode_preamble() thus > > effectively casts u32 -> int but the checks for segment lengths are > > written as if on unsigned values. While reading in HELLO or one of the > > AUTH frames (before authentication is completed), arithmetic in > > head_onwire_len() can get duped by negative ctrl_len and produce > > head_len which is less than CEPH_PREAMBLE_LEN but still positive. > > This would lead to a buffer overrun in prepare_read_control() as the > > preamble gets copied to the newly allocated buffer of size head_len. > > > > Cc: stable@vger.kernel.org > > Fixes: cd1a677cad99 ("libceph, ceph: implement msgr2.1 protocol (crc and secure modes)") > > Reported-by: Thelford Williams <thelford@google.com> > > Signed-off-by: Ilya Dryomov <idryomov@gmail.com> > > --- > > net/ceph/messenger_v2.c | 41 ++++++++++++++++++++++++++--------------- > > 1 file changed, 26 insertions(+), 15 deletions(-) > > > > diff --git a/net/ceph/messenger_v2.c b/net/ceph/messenger_v2.c > > index 1a888b86a494..1df1d29dee92 100644 > > --- a/net/ceph/messenger_v2.c > > +++ b/net/ceph/messenger_v2.c > > @@ -390,6 +390,8 @@ static int head_onwire_len(int ctrl_len, bool secure) > > int head_len; > > int rem_len; > > > > + BUG_ON(ctrl_len < 0 || ctrl_len > CEPH_MSG_MAX_CONTROL_LEN); > > Doesn't the ctrl_len ultimately come from the outside? If so > you should not do BUG_ON() with bad values. It does, but it's now checked in decode_preamble(). This is a secondary defense for which BUG_ON feels appropriate. Thanks, Ilya
On 7/12/23 20:07, Ilya Dryomov wrote: > ceph_frame_desc::fd_lens is an int array. decode_preamble() thus > effectively casts u32 -> int but the checks for segment lengths are > written as if on unsigned values. While reading in HELLO or one of the > AUTH frames (before authentication is completed), arithmetic in > head_onwire_len() can get duped by negative ctrl_len and produce > head_len which is less than CEPH_PREAMBLE_LEN but still positive. > This would lead to a buffer overrun in prepare_read_control() as the > preamble gets copied to the newly allocated buffer of size head_len. > > Cc: stable@vger.kernel.org > Fixes: cd1a677cad99 ("libceph, ceph: implement msgr2.1 protocol (crc and secure modes)") > Reported-by: Thelford Williams <thelford@google.com> > Signed-off-by: Ilya Dryomov <idryomov@gmail.com> > --- > net/ceph/messenger_v2.c | 41 ++++++++++++++++++++++++++--------------- > 1 file changed, 26 insertions(+), 15 deletions(-) > > diff --git a/net/ceph/messenger_v2.c b/net/ceph/messenger_v2.c > index 1a888b86a494..1df1d29dee92 100644 > --- a/net/ceph/messenger_v2.c > +++ b/net/ceph/messenger_v2.c > @@ -390,6 +390,8 @@ static int head_onwire_len(int ctrl_len, bool secure) > int head_len; > int rem_len; > > + BUG_ON(ctrl_len < 0 || ctrl_len > CEPH_MSG_MAX_CONTROL_LEN); > + > if (secure) { > head_len = CEPH_PREAMBLE_SECURE_LEN; > if (ctrl_len > CEPH_PREAMBLE_INLINE_LEN) { > @@ -408,6 +410,10 @@ static int head_onwire_len(int ctrl_len, bool secure) > static int __tail_onwire_len(int front_len, int middle_len, int data_len, > bool secure) > { > + BUG_ON(front_len < 0 || front_len > CEPH_MSG_MAX_FRONT_LEN || > + middle_len < 0 || middle_len > CEPH_MSG_MAX_MIDDLE_LEN || > + data_len < 0 || data_len > CEPH_MSG_MAX_DATA_LEN); > + > if (!front_len && !middle_len && !data_len) > return 0; > > @@ -520,29 +526,34 @@ static int decode_preamble(void *p, struct ceph_frame_desc *desc) > desc->fd_aligns[i] = ceph_decode_16(&p); > } > > - /* > - * This would fire for FRAME_TAG_WAIT (it has one empty > - * segment), but we should never get it as client. > - */ > - if (!desc->fd_lens[desc->fd_seg_cnt - 1]) { > - pr_err("last segment empty\n"); > + if (desc->fd_lens[0] < 0 || > + desc->fd_lens[0] > CEPH_MSG_MAX_CONTROL_LEN) { > + pr_err("bad control segment length %d\n", desc->fd_lens[0]); > return -EINVAL; > } > - > - if (desc->fd_lens[0] > CEPH_MSG_MAX_CONTROL_LEN) { > - pr_err("control segment too big %d\n", desc->fd_lens[0]); > + if (desc->fd_lens[1] < 0 || > + desc->fd_lens[1] > CEPH_MSG_MAX_FRONT_LEN) { > + pr_err("bad front segment length %d\n", desc->fd_lens[1]); > return -EINVAL; > } > - if (desc->fd_lens[1] > CEPH_MSG_MAX_FRONT_LEN) { > - pr_err("front segment too big %d\n", desc->fd_lens[1]); > + if (desc->fd_lens[2] < 0 || > + desc->fd_lens[2] > CEPH_MSG_MAX_MIDDLE_LEN) { > + pr_err("bad middle segment length %d\n", desc->fd_lens[2]); > return -EINVAL; > } > - if (desc->fd_lens[2] > CEPH_MSG_MAX_MIDDLE_LEN) { > - pr_err("middle segment too big %d\n", desc->fd_lens[2]); > + if (desc->fd_lens[3] < 0 || > + desc->fd_lens[3] > CEPH_MSG_MAX_DATA_LEN) { > + pr_err("bad data segment length %d\n", desc->fd_lens[3]); > return -EINVAL; > } > - if (desc->fd_lens[3] > CEPH_MSG_MAX_DATA_LEN) { > - pr_err("data segment too big %d\n", desc->fd_lens[3]); > + > + /* > + * This would fire for FRAME_TAG_WAIT (it has one empty > + * segment), but we should never get it as client. > + */ > + if (!desc->fd_lens[desc->fd_seg_cnt - 1]) { > + pr_err("last segment empty, segment count %d\n", > + desc->fd_seg_cnt); > return -EINVAL; > } > LGTM. Reviewed-by: Xiubo Li <xiubli@redhat.com>
diff --git a/net/ceph/messenger_v2.c b/net/ceph/messenger_v2.c index 1a888b86a494..1df1d29dee92 100644 --- a/net/ceph/messenger_v2.c +++ b/net/ceph/messenger_v2.c @@ -390,6 +390,8 @@ static int head_onwire_len(int ctrl_len, bool secure) int head_len; int rem_len; + BUG_ON(ctrl_len < 0 || ctrl_len > CEPH_MSG_MAX_CONTROL_LEN); + if (secure) { head_len = CEPH_PREAMBLE_SECURE_LEN; if (ctrl_len > CEPH_PREAMBLE_INLINE_LEN) { @@ -408,6 +410,10 @@ static int head_onwire_len(int ctrl_len, bool secure) static int __tail_onwire_len(int front_len, int middle_len, int data_len, bool secure) { + BUG_ON(front_len < 0 || front_len > CEPH_MSG_MAX_FRONT_LEN || + middle_len < 0 || middle_len > CEPH_MSG_MAX_MIDDLE_LEN || + data_len < 0 || data_len > CEPH_MSG_MAX_DATA_LEN); + if (!front_len && !middle_len && !data_len) return 0; @@ -520,29 +526,34 @@ static int decode_preamble(void *p, struct ceph_frame_desc *desc) desc->fd_aligns[i] = ceph_decode_16(&p); } - /* - * This would fire for FRAME_TAG_WAIT (it has one empty - * segment), but we should never get it as client. - */ - if (!desc->fd_lens[desc->fd_seg_cnt - 1]) { - pr_err("last segment empty\n"); + if (desc->fd_lens[0] < 0 || + desc->fd_lens[0] > CEPH_MSG_MAX_CONTROL_LEN) { + pr_err("bad control segment length %d\n", desc->fd_lens[0]); return -EINVAL; } - - if (desc->fd_lens[0] > CEPH_MSG_MAX_CONTROL_LEN) { - pr_err("control segment too big %d\n", desc->fd_lens[0]); + if (desc->fd_lens[1] < 0 || + desc->fd_lens[1] > CEPH_MSG_MAX_FRONT_LEN) { + pr_err("bad front segment length %d\n", desc->fd_lens[1]); return -EINVAL; } - if (desc->fd_lens[1] > CEPH_MSG_MAX_FRONT_LEN) { - pr_err("front segment too big %d\n", desc->fd_lens[1]); + if (desc->fd_lens[2] < 0 || + desc->fd_lens[2] > CEPH_MSG_MAX_MIDDLE_LEN) { + pr_err("bad middle segment length %d\n", desc->fd_lens[2]); return -EINVAL; } - if (desc->fd_lens[2] > CEPH_MSG_MAX_MIDDLE_LEN) { - pr_err("middle segment too big %d\n", desc->fd_lens[2]); + if (desc->fd_lens[3] < 0 || + desc->fd_lens[3] > CEPH_MSG_MAX_DATA_LEN) { + pr_err("bad data segment length %d\n", desc->fd_lens[3]); return -EINVAL; } - if (desc->fd_lens[3] > CEPH_MSG_MAX_DATA_LEN) { - pr_err("data segment too big %d\n", desc->fd_lens[3]); + + /* + * This would fire for FRAME_TAG_WAIT (it has one empty + * segment), but we should never get it as client. + */ + if (!desc->fd_lens[desc->fd_seg_cnt - 1]) { + pr_err("last segment empty, segment count %d\n", + desc->fd_seg_cnt); return -EINVAL; }
ceph_frame_desc::fd_lens is an int array. decode_preamble() thus effectively casts u32 -> int but the checks for segment lengths are written as if on unsigned values. While reading in HELLO or one of the AUTH frames (before authentication is completed), arithmetic in head_onwire_len() can get duped by negative ctrl_len and produce head_len which is less than CEPH_PREAMBLE_LEN but still positive. This would lead to a buffer overrun in prepare_read_control() as the preamble gets copied to the newly allocated buffer of size head_len. Cc: stable@vger.kernel.org Fixes: cd1a677cad99 ("libceph, ceph: implement msgr2.1 protocol (crc and secure modes)") Reported-by: Thelford Williams <thelford@google.com> Signed-off-by: Ilya Dryomov <idryomov@gmail.com> --- net/ceph/messenger_v2.c | 41 ++++++++++++++++++++++++++--------------- 1 file changed, 26 insertions(+), 15 deletions(-)