On Wed, Jul 12, 2023 at 2:34 PM Alex Elder <elder@xxxxxxxx> 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@xxxxxxxxxxxxxxx
> > Fixes: cd1a677cad99 ("libceph, ceph: implement msgr2.1 protocol (crc and secure modes)")
> > Reported-by: Thelford Williams <thelford@xxxxxxxxxx>
> > Signed-off-by: Ilya Dryomov <idryomov@xxxxxxxxx>
> > ---
> > 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
