Re: [PATCH] libceph: Fix multiplication overflow in __decode_pg_upmap_items()

Re: [PATCH] libceph: Fix multiplication overflow in __decode_pg_upmap_items()

[Viacheslav Dubeyko]

cc: ceph-devel@vger.kernel.org

On Wed, 2026-05-13 at 10:14 +0200, Raphael Zimmer wrote:
> A message of type CEPH_MSG_OSD_MAP holds an OSD map, which typically
> contains a pg_upmap part at its end. When decoding this part in
> __decode_pg_upmap_items(), a len value is decoded from the message to
> determine the number of items and the size of the allocation needed
> for
> them. If the len value is greater than or equal to 2^31, an overflow
> occurs in the multiplication that is performed to determine the
> needed
> size of the incoming buffer to decode, as well as for the length of
> the
> allocation for the ceph_pg_mapping struct. Subsequently, this results
> in
> out-of-bounds writes (and reads) when decoding the incoming message
> fields into the ceph_pg_mapping struct.
> 
> This patch fixes the issue by adding a UL suffix to the literal in
> the
> multiplication to perform it as an unsigned long multiplication.
> 
> Signed-off-by: Raphael Zimmer <raphael.zimmer@tu-ilmenau.de>
> ---
>  net/ceph/osdmap.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/net/ceph/osdmap.c b/net/ceph/osdmap.c
> index 8b5b0587a0cf..42b7b5300901 100644
> --- a/net/ceph/osdmap.c
> +++ b/net/ceph/osdmap.c
> @@ -1620,8 +1620,8 @@ static struct ceph_pg_mapping
> *__decode_pg_upmap_items(void **p, void *end,
>  	if ((size_t)len > (SIZE_MAX - sizeof(*pg)) / (2 *
> sizeof(u32)))
>  		return ERR_PTR(-EINVAL);
>  
> -	ceph_decode_need(p, end, 2 * len * sizeof(u32), e_inval);
> -	pg = alloc_pg_mapping(2 * len * sizeof(u32));
> +	ceph_decode_need(p, end, 2UL * len * sizeof(u32), e_inval);
> +	pg = alloc_pg_mapping(2UL * len * sizeof(u32));
>  	if (!pg)
>  		return ERR_PTR(-ENOMEM);
>  

Re: [bug report] libceph: Multiplication overflow that leads to out-of-bounds writes in __decode_pg_upmap_items()

[Viacheslav Dubeyko]

cc: ceph-devel@vger.kernel.org

On Wed, 2026-05-13 at 10:13 +0200, Raphael Zimmer wrote:
> Hi,
> I encountered a bug in the libceph module, specifically in the
> function
> __decode_pg_upmap_items() in net/ceph/osdmap.c. It can be triggered
> by a
> message of type CEPH_MSG_OSD_MAP arriving on an open connection to a
> Ceph monitor or OSD.
> 
> A message of type CEPH_MSG_OSD_MAP holds an OSD map, which typically
> contains a pg_upmap part at its end. When decoding this part,
> __decode_pg_upmap_items() is called. In this function, a len value is
> decoded from the message. It is used to determine the number of items
> and the size of the allocation needed for them. If the len value is
> greater than or equal to 2^31, an overflow occurs in the
> multiplication
> that is performed to determine the needed size of the incoming buffer
> to
> decode, as well as for the length of the allocation for the
> ceph_pg_mapping struct. The performed multiplication is "2 * len *
> sizeof(u32)" where len is of type u32. Subsequently, the len
> parameter
> is used as the upper bound of a loop that decodes the incoming
> message
> fields into the ceph_pg_mapping struct. This results in out-of-bounds
> memory accesses in case of a previous overflow. The len field can be
> set
> arbitrarily by a (maliciously acting) Ceph monitor or OSD and allows
> writing data received over the network beyond the end of the
> allocated
> buffer. Furthermore, I observed that it can ultimately result in a
> kernel panic because of general protection faults when attempting to
> access the memory.
> 
> The issues can be reproduced on the recent stable release 7.0.6 when
> injecting a specifically crafted message into an open Ceph
> connection.
> However, 32-bit systems are probably not affected, as the previous
> check
> for:
> if ((size_t)len > (SIZE_MAX - sizeof(*pg)) / (2 * sizeof(u32)))
> 		return ERR_PTR(-EINVAL);
> would not allow a value this large for len on these systems.
> 
> The bug can be fixed by explicitly performing the multiplication in
> 64
> bit on 64-bit systems by switching to "2UL * len * sizeof(u32)" or
> changing the evaluation order of the multiplication. This would
> suffice
> as the mentioned check catches such values already beforehand on 32-
> bit
> systems, so that an additional check is not necessary. However, I
> would
> also be open to adding an additional check instead if it seems more
> appropriate.
> 
> I will send the proposed fix in reply to this email.
> 
> 
> Best regards,
> Raphael

RE: [PATCH] libceph: Fix multiplication overflow in __decode_pg_upmap_items()

[Viacheslav Dubeyko]

On Wed, 2026-05-13 at 09:40 -0700, Viacheslav Dubeyko wrote:
> cc: ceph-devel@vger.kernel.org
> 
> On Wed, 2026-05-13 at 10:14 +0200, Raphael Zimmer wrote:
> > A message of type CEPH_MSG_OSD_MAP holds an OSD map, which typically
> > contains a pg_upmap part at its end. When decoding this part in
> > __decode_pg_upmap_items(), a len value is decoded from the message to
> > determine the number of items and the size of the allocation needed
> > for
> > them. If the len value is greater than or equal to 2^31, an overflow
> > occurs in the multiplication that is performed to determine the
> > needed
> > size of the incoming buffer to decode, as well as for the length of
> > the
> > allocation for the ceph_pg_mapping struct. Subsequently, this results
> > in
> > out-of-bounds writes (and reads) when decoding the incoming message
> > fields into the ceph_pg_mapping struct.
> > 
> > This patch fixes the issue by adding a UL suffix to the literal in
> > the
> > multiplication to perform it as an unsigned long multiplication.
> > 
> > Signed-off-by: Raphael Zimmer <raphael.zimmer@tu-ilmenau.de>
> > ---
> >  net/ceph/osdmap.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/net/ceph/osdmap.c b/net/ceph/osdmap.c
> > index 8b5b0587a0cf..42b7b5300901 100644
> > --- a/net/ceph/osdmap.c
> > +++ b/net/ceph/osdmap.c
> > @@ -1620,8 +1620,8 @@ static struct ceph_pg_mapping
> > *__decode_pg_upmap_items(void **p, void *end,
> >  	if ((size_t)len > (SIZE_MAX - sizeof(*pg)) / (2 *
> > sizeof(u32)))
> >  		return ERR_PTR(-EINVAL);
> >  
> > -	ceph_decode_need(p, end, 2 * len * sizeof(u32), e_inval);

We use the pattern 2 * sizeof(u32) three times in the function. I can suggest to
introduce a local variable that will keep this value.

> > -	pg = alloc_pg_mapping(2 * len * sizeof(u32));

I think it doesn't make sense to execute the same calculation two times. Of
course, compiler can do some optimizations. But I suggest to introduce a local
variable that will contain the final calculation of 2UL * len * sizeof(u32) and
it will be used in both cases.

> > +	ceph_decode_need(p, end, 2UL * len * sizeof(u32), e_inval);
> > +	pg = alloc_pg_mapping(2UL * len * sizeof(u32));
> >  	if (!pg)
> >  		return ERR_PTR(-ENOMEM);
> >  

By the way, this check:

	if ((size_t)len > (SIZE_MAX - sizeof(*pg)) / (2 * sizeof(u32)))
		return ERR_PTR(-EINVAL);

will work  a wrong way for the case of if the len value is greater than or equal
to 2^31.

Thanks,
Slava.

Re: [PATCH] libceph: Fix multiplication overflow in __decode_pg_upmap_items()

[Raphael Zimmer]

On 13.05.26 7:18 PM, Viacheslav Dubeyko wrote:
> On Wed, 2026-05-13 at 09:40 -0700, Viacheslav Dubeyko wrote:
>> cc: ceph-devel@vger.kernel.org
>>
>> On Wed, 2026-05-13 at 10:14 +0200, Raphael Zimmer wrote:
>>> A message of type CEPH_MSG_OSD_MAP holds an OSD map, which typically
>>> contains a pg_upmap part at its end. When decoding this part in
>>> __decode_pg_upmap_items(), a len value is decoded from the message to
>>> determine the number of items and the size of the allocation needed
>>> for
>>> them. If the len value is greater than or equal to 2^31, an overflow
>>> occurs in the multiplication that is performed to determine the
>>> needed
>>> size of the incoming buffer to decode, as well as for the length of
>>> the
>>> allocation for the ceph_pg_mapping struct. Subsequently, this results
>>> in
>>> out-of-bounds writes (and reads) when decoding the incoming message
>>> fields into the ceph_pg_mapping struct.
>>>
>>> This patch fixes the issue by adding a UL suffix to the literal in
>>> the
>>> multiplication to perform it as an unsigned long multiplication.
>>>
>>> Signed-off-by: Raphael Zimmer <raphael.zimmer@tu-ilmenau.de>
>>> ---
>>>  net/ceph/osdmap.c | 4 ++--
>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/net/ceph/osdmap.c b/net/ceph/osdmap.c
>>> index 8b5b0587a0cf..42b7b5300901 100644
>>> --- a/net/ceph/osdmap.c
>>> +++ b/net/ceph/osdmap.c
>>> @@ -1620,8 +1620,8 @@ static struct ceph_pg_mapping
>>> *__decode_pg_upmap_items(void **p, void *end,
>>>  	if ((size_t)len > (SIZE_MAX - sizeof(*pg)) / (2 *
>>> sizeof(u32)))
>>>  		return ERR_PTR(-EINVAL);
>>>  
>>> -	ceph_decode_need(p, end, 2 * len * sizeof(u32), e_inval);
> 
> We use the pattern 2 * sizeof(u32) three times in the function. I can suggest to
> introduce a local variable that will keep this value.
> 
>>> -	pg = alloc_pg_mapping(2 * len * sizeof(u32));
> 
> I think it doesn't make sense to execute the same calculation two times. Of
> course, compiler can do some optimizations. But I suggest to introduce a local
> variable that will contain the final calculation of 2UL * len * sizeof(u32) and
> it will be used in both cases.
> 

Hi Slava,
I will rework this.

>>> +	ceph_decode_need(p, end, 2UL * len * sizeof(u32), e_inval);
>>> +	pg = alloc_pg_mapping(2UL * len * sizeof(u32));
>>>  	if (!pg)
>>>  		return ERR_PTR(-ENOMEM);
>>>  
> 
> By the way, this check:
> 
> 	if ((size_t)len > (SIZE_MAX - sizeof(*pg)) / (2 * sizeof(u32)))
> 		return ERR_PTR(-EINVAL);
> 
> will work  a wrong way for the case of if the len value is greater than or equal
> to 2^31.
> 

What do you mean by "will work a wrong way"? I don't see any issue with
it or different behavior in case of len >= 2^31.

Best regards,
Raphael

> Thanks,
> Slava.

RE: [PATCH] libceph: Fix multiplication overflow in __decode_pg_upmap_items()

[Viacheslav Dubeyko]

On Fri, 2026-05-15 at 09:41 +0200, Raphael Zimmer wrote:
> On 13.05.26 7:18 PM, Viacheslav Dubeyko wrote:
> > On Wed, 2026-05-13 at 09:40 -0700, Viacheslav Dubeyko wrote:
> > > cc: ceph-devel@vger.kernel.org
> > > 
> > > On Wed, 2026-05-13 at 10:14 +0200, Raphael Zimmer wrote:
> > > > A message of type CEPH_MSG_OSD_MAP holds an OSD map, which typically
> > > > contains a pg_upmap part at its end. When decoding this part in
> > > > __decode_pg_upmap_items(), a len value is decoded from the message to
> > > > determine the number of items and the size of the allocation needed
> > > > for
> > > > them. If the len value is greater than or equal to 2^31, an overflow
> > > > occurs in the multiplication that is performed to determine the
> > > > needed
> > > > size of the incoming buffer to decode, as well as for the length of
> > > > the
> > > > allocation for the ceph_pg_mapping struct. Subsequently, this results
> > > > in
> > > > out-of-bounds writes (and reads) when decoding the incoming message
> > > > fields into the ceph_pg_mapping struct.
> > > > 
> > > > This patch fixes the issue by adding a UL suffix to the literal in
> > > > the
> > > > multiplication to perform it as an unsigned long multiplication.
> > > > 
> > > > Signed-off-by: Raphael Zimmer <raphael.zimmer@tu-ilmenau.de>
> > > > ---
> > > >  net/ceph/osdmap.c | 4 ++--
> > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/net/ceph/osdmap.c b/net/ceph/osdmap.c
> > > > index 8b5b0587a0cf..42b7b5300901 100644
> > > > --- a/net/ceph/osdmap.c
> > > > +++ b/net/ceph/osdmap.c
> > > > @@ -1620,8 +1620,8 @@ static struct ceph_pg_mapping
> > > > *__decode_pg_upmap_items(void **p, void *end,
> > > >  	if ((size_t)len > (SIZE_MAX - sizeof(*pg)) / (2 *
> > > > sizeof(u32)))
> > > >  		return ERR_PTR(-EINVAL);
> > > >  
> > > > -	ceph_decode_need(p, end, 2 * len * sizeof(u32), e_inval);
> > 
> > We use the pattern 2 * sizeof(u32) three times in the function. I can suggest to
> > introduce a local variable that will keep this value.
> > 
> > > > -	pg = alloc_pg_mapping(2 * len * sizeof(u32));
> > 
> > I think it doesn't make sense to execute the same calculation two times. Of
> > course, compiler can do some optimizations. But I suggest to introduce a local
> > variable that will contain the final calculation of 2UL * len * sizeof(u32) and
> > it will be used in both cases.
> > 
> 
> Hi Slava,
> I will rework this.
> 
> > > > +	ceph_decode_need(p, end, 2UL * len * sizeof(u32), e_inval);
> > > > +	pg = alloc_pg_mapping(2UL * len * sizeof(u32));
> > > >  	if (!pg)
> > > >  		return ERR_PTR(-ENOMEM);
> > > >  
> > 
> > By the way, this check:
> > 
> > 	if ((size_t)len > (SIZE_MAX - sizeof(*pg)) / (2 * sizeof(u32)))
> > 		return ERR_PTR(-EINVAL);
> > 
> > will work  a wrong way for the case of if the len value is greater than or equal
> > to 2^31.
> > 
> 
> What do you mean by "will work a wrong way"? I don't see any issue with
> it or different behavior in case of len >= 2^31.
> 
> 

I think the main confusing point was this phrase from the commit message "If the
len value is greater than or equal to 2^31". But, as far as I can see, we have
declaration [1]:

u32 len, i;

Then, how the len can have value "greater than 2^31"? :) I assume that you mean
the calculation of 2 * len * sizeof(u32) can be greater than 2^31.

But, maybe, we need to have this declaration instead:

size_t len, i;

In such case, we don't need in the case (size_t)len here:

if ((size_t)len > (SIZE_MAX - sizeof(*pg)) / (2 * sizeof(u32)))

What do you think?

Thanks,
Slava. 

[1] https://elixir.bootlin.com/linux/v7.1-rc3/source/net/ceph/osdmap.c#L1610