Re: CVE-2023-52685: pstore: ram_core: fix possible overflow in persistent_ram_init_ecc()

Re: CVE-2023-52685: pstore: ram_core: fix possible overflow in persistent_ram_init_ecc()

[Gabriel Krisman Bertazi]

Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes:

> Description
> ===========
>
> In the Linux kernel, the following vulnerability has been resolved:
>
> pstore: ram_core: fix possible overflow in persistent_ram_init_ecc()
>
> In persistent_ram_init_ecc(), on 64-bit arches DIV_ROUND_UP() will return
> 64-bit value since persistent_ram_zone::buffer_size has type size_t which
> is derived from the 64-bit *unsigned long*, while the ecc_blocks variable
> this value gets assigned to has (always 32-bit) *int* type.  Even if that
> value fits into *int* type, an overflow is still possible when calculating
> the size_t typed ecc_total variable further below since there's no cast to
> any 64-bit type before multiplication.  Declaring the ecc_blocks variable
> as *size_t* should fix this mess...
>
> Found by Linux Verification Center (linuxtesting.org) with the SVACE static
> analysis tool.

Hi Greg,

[Cc'ing Kees, who is listed as the pstore maintainer]

I want to dispute this CVE.  The overflow is in the module
initialization path, and can only happen at boot time or if the module
is loaded with specific parameters or due to specific acpi/device tree
data.  Either way, it would require root privileges to trigger.

-- 
Gabriel Krisman Bertazi

Re: CVE-2023-52685: pstore: ram_core: fix possible overflow in persistent_ram_init_ecc()

[Greg Kroah-Hartman]

On Mon, May 27, 2024 at 08:32:54PM -0400, Gabriel Krisman Bertazi wrote:
> Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes:
> 
> > Description
> > ===========
> >
> > In the Linux kernel, the following vulnerability has been resolved:
> >
> > pstore: ram_core: fix possible overflow in persistent_ram_init_ecc()
> >
> > In persistent_ram_init_ecc(), on 64-bit arches DIV_ROUND_UP() will return
> > 64-bit value since persistent_ram_zone::buffer_size has type size_t which
> > is derived from the 64-bit *unsigned long*, while the ecc_blocks variable
> > this value gets assigned to has (always 32-bit) *int* type.  Even if that
> > value fits into *int* type, an overflow is still possible when calculating
> > the size_t typed ecc_total variable further below since there's no cast to
> > any 64-bit type before multiplication.  Declaring the ecc_blocks variable
> > as *size_t* should fix this mess...
> >
> > Found by Linux Verification Center (linuxtesting.org) with the SVACE static
> > analysis tool.
> 
> Hi Greg,
> 
> [Cc'ing Kees, who is listed as the pstore maintainer]
> 
> I want to dispute this CVE.  The overflow is in the module
> initialization path, and can only happen at boot time or if the module
> is loaded with specific parameters or due to specific acpi/device tree
> data.  Either way, it would require root privileges to trigger.

Normally root privileges isn't the issue, as many containers allow root
to do things (including loading modules, crazy systems...)

Anyway, I'll defer to Kees as to if this should be revoked or not.

thanks,

gre gk-h

> 
> -- 
> Gabriel Krisman Bertazi

Re: CVE-2023-52685: pstore: ram_core: fix possible overflow in persistent_ram_init_ecc()

[Kees Cook]

On Tue, May 28, 2024 at 09:01:13PM +0200, Greg Kroah-Hartman wrote:
> On Mon, May 27, 2024 at 08:32:54PM -0400, Gabriel Krisman Bertazi wrote:
> > Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes:
> > 
> > > Description
> > > ===========
> > >
> > > In the Linux kernel, the following vulnerability has been resolved:
> > >
> > > pstore: ram_core: fix possible overflow in persistent_ram_init_ecc()
> > >
> > > In persistent_ram_init_ecc(), on 64-bit arches DIV_ROUND_UP() will return
> > > 64-bit value since persistent_ram_zone::buffer_size has type size_t which
> > > is derived from the 64-bit *unsigned long*, while the ecc_blocks variable
> > > this value gets assigned to has (always 32-bit) *int* type.  Even if that
> > > value fits into *int* type, an overflow is still possible when calculating
> > > the size_t typed ecc_total variable further below since there's no cast to
> > > any 64-bit type before multiplication.  Declaring the ecc_blocks variable
> > > as *size_t* should fix this mess...
> > >
> > > Found by Linux Verification Center (linuxtesting.org) with the SVACE static
> > > analysis tool.
> > 
> > Hi Greg,
> > 
> > [Cc'ing Kees, who is listed as the pstore maintainer]
> > 
> > I want to dispute this CVE.  The overflow is in the module
> > initialization path, and can only happen at boot time or if the module
> > is loaded with specific parameters or due to specific acpi/device tree
> > data.  Either way, it would require root privileges to trigger.
> 
> Normally root privileges isn't the issue, as many containers allow root
> to do things (including loading modules, crazy systems...)
> 
> Anyway, I'll defer to Kees as to if this should be revoked or not.

It's a module parameter or device tree value that is at most INT_MAX or
UINT_MAX respectively. Also, it is bounds checked against the buffer
itself:
        if (ecc_total >= prz->buffer_size) {

So even if it wrapped around and got "too small", there's no damage to
be had here.

The worst case is that the ramoops info goes missing because pstore
refuses to do anything with the bad value, but pstore can be disabled
way more easily than that, by design.

So, no, I don't think this is CVE worthy. I took the patch because it's
reasonable to try to get the math right and provide better error
reporting.

-- 
Kees Cook

Re: CVE-2023-52685: pstore: ram_core: fix possible overflow in persistent_ram_init_ecc()

[Greg Kroah-Hartman]

On Mon, Jun 17, 2024 at 02:17:49PM -0700, Kees Cook wrote:
> On Tue, May 28, 2024 at 09:01:13PM +0200, Greg Kroah-Hartman wrote:
> > On Mon, May 27, 2024 at 08:32:54PM -0400, Gabriel Krisman Bertazi wrote:
> > > Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes:
> > > 
> > > > Description
> > > > ===========
> > > >
> > > > In the Linux kernel, the following vulnerability has been resolved:
> > > >
> > > > pstore: ram_core: fix possible overflow in persistent_ram_init_ecc()
> > > >
> > > > In persistent_ram_init_ecc(), on 64-bit arches DIV_ROUND_UP() will return
> > > > 64-bit value since persistent_ram_zone::buffer_size has type size_t which
> > > > is derived from the 64-bit *unsigned long*, while the ecc_blocks variable
> > > > this value gets assigned to has (always 32-bit) *int* type.  Even if that
> > > > value fits into *int* type, an overflow is still possible when calculating
> > > > the size_t typed ecc_total variable further below since there's no cast to
> > > > any 64-bit type before multiplication.  Declaring the ecc_blocks variable
> > > > as *size_t* should fix this mess...
> > > >
> > > > Found by Linux Verification Center (linuxtesting.org) with the SVACE static
> > > > analysis tool.
> > > 
> > > Hi Greg,
> > > 
> > > [Cc'ing Kees, who is listed as the pstore maintainer]
> > > 
> > > I want to dispute this CVE.  The overflow is in the module
> > > initialization path, and can only happen at boot time or if the module
> > > is loaded with specific parameters or due to specific acpi/device tree
> > > data.  Either way, it would require root privileges to trigger.
> > 
> > Normally root privileges isn't the issue, as many containers allow root
> > to do things (including loading modules, crazy systems...)
> > 
> > Anyway, I'll defer to Kees as to if this should be revoked or not.
> 
> It's a module parameter or device tree value that is at most INT_MAX or
> UINT_MAX respectively. Also, it is bounds checked against the buffer
> itself:
>         if (ecc_total >= prz->buffer_size) {
> 
> So even if it wrapped around and got "too small", there's no damage to
> be had here.
> 
> The worst case is that the ramoops info goes missing because pstore
> refuses to do anything with the bad value, but pstore can be disabled
> way more easily than that, by design.
> 
> So, no, I don't think this is CVE worthy. I took the patch because it's
> reasonable to try to get the math right and provide better error
> reporting.

Now rejected, thanks.

greg k-h