validate_section_offset() uses unsigned long local variable to
add/store shdr->sh_offset and shdr->sh_size on all platforms.
unsigned long is too short when sh_offset is Elf64_Off which
would be the case on 64bit ELF headers.
This problem was found while adding an error message to print
sh_offset and sh_size. If sh_offset + sh_size exceed the size
of the local variable, the checks for overflow and offset/size
being too large will not find the problem and call the section
offset valid. This failure might cause problems later on.
Fix the overflow problem using the right size local variable when
CONFIG_64BIT is defined.
Signed-off-by: Shuah Khan <[email protected]>
---
Changes since v1:
- Updated commit log to describe the fix clearly. No code
changes.
kernel/module.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/kernel/module.c b/kernel/module.c
index ad03a2357377..84a9141a5e15 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -2942,7 +2942,11 @@ static int module_sig_check(struct load_info *info, int flags)
static int validate_section_offset(struct load_info *info, Elf_Shdr *shdr)
{
+#if defined(CONFIG_64BIT)
+ unsigned long long secend;
+#else
unsigned long secend;
+#endif
/*
* Check for both overflow and offset/size being
--
2.30.2
On Mon, Oct 18, 2021 at 11:35:11AM -0600, Shuah Khan wrote:
> validate_section_offset() uses unsigned long local variable to
> add/store shdr->sh_offset and shdr->sh_size on all platforms.
> unsigned long is too short when sh_offset is Elf64_Off which
> would be the case on 64bit ELF headers.
>
> This problem was found while adding an error message to print
> sh_offset and sh_size. If sh_offset + sh_size exceed the size
> of the local variable, the checks for overflow and offset/size
> being too large will not find the problem and call the section
> offset valid. This failure might cause problems later on.
>
> Fix the overflow problem using the right size local variable when
> CONFIG_64BIT is defined.
>
> Signed-off-by: Shuah Khan <[email protected]>
> ---
> Changes since v1:
> - Updated commit log to describe the fix clearly. No code
> changes.
Thanks! But the implications of your fix is beyond what is described.
Although not a real issue today in practice.
I think we should extend it with something like this, let me know
what you think (I can just ammend the commit log, no resend would
be needed):
Without this fix applied we were shorting the design of modules to
have section headers placed within the 32-bit boundary (4 GiB) instead of
64-bits when on 64-bit architectures (which allows for up to 16,777,216
TiB). In practice this just meant we were limiting modules to below
4 GiB even on 64-bit systems. This then should not really affect any
real-world use case as modules these days obviously should likely never
exceed 1 GiB in size. A specially crafted invalid module might succeed to
skip validation in validate_section_offset() due to this mistake, but in such
case no impact is observed through code inspection given the correct data
types are used for the copy of the module when needed on move_module() when
the section type is not SHT_NOBITS (which indicates no the section
occupies no space on the file).
Luis
> kernel/module.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/kernel/module.c b/kernel/module.c
> index ad03a2357377..84a9141a5e15 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -2942,7 +2942,11 @@ static int module_sig_check(struct load_info *info, int flags)
>
> static int validate_section_offset(struct load_info *info, Elf_Shdr *shdr)
> {
> +#if defined(CONFIG_64BIT)
> + unsigned long long secend;
> +#else
> unsigned long secend;
> +#endif
>
> /*
> * Check for both overflow and offset/size being
> --
> 2.30.2
>
On 10/18/21 2:20 PM, Luis Chamberlain wrote:
> On Mon, Oct 18, 2021 at 11:35:11AM -0600, Shuah Khan wrote:
>> validate_section_offset() uses unsigned long local variable to
>> add/store shdr->sh_offset and shdr->sh_size on all platforms.
>> unsigned long is too short when sh_offset is Elf64_Off which
>> would be the case on 64bit ELF headers.
>>
>> This problem was found while adding an error message to print
>> sh_offset and sh_size. If sh_offset + sh_size exceed the size
>> of the local variable, the checks for overflow and offset/size
>> being too large will not find the problem and call the section
>> offset valid. This failure might cause problems later on.
>>
>> Fix the overflow problem using the right size local variable when
>> CONFIG_64BIT is defined.
>>
>> Signed-off-by: Shuah Khan <[email protected]>
>> ---
>> Changes since v1:
>> - Updated commit log to describe the fix clearly. No code
>> changes.
>
> Thanks! But the implications of your fix is beyond what is described.
> Although not a real issue today in practice.
>
> I think we should extend it with something like this, let me know
> what you think (I can just ammend the commit log, no resend would
> be needed):
>
> Without this fix applied we were shorting the design of modules to
> have section headers placed within the 32-bit boundary (4 GiB) instead of
> 64-bits when on 64-bit architectures (which allows for up to 16,777,216
> TiB). In practice this just meant we were limiting modules to below
> 4 GiB even on 64-bit systems. This then should not really affect any
> real-world use case as modules these days obviously should likely never
> exceed 1 GiB in size. A specially crafted invalid module might succeed to
> skip validation in validate_section_offset() due to this mistake, but in such
> case no impact is observed through code inspection given the correct data
> types are used for the copy of the module when needed on move_module() when
> the section type is not SHT_NOBITS (which indicates no the section
> occupies no space on the file).
>
Sounds good to me.
thanks,
-- Shuah
On Mon, Oct 18, 2021 at 02:57:45PM -0600, Shuah Khan wrote:
> On 10/18/21 2:20 PM, Luis Chamberlain wrote:
> > On Mon, Oct 18, 2021 at 11:35:11AM -0600, Shuah Khan wrote:
> > > validate_section_offset() uses unsigned long local variable to
> > > add/store shdr->sh_offset and shdr->sh_size on all platforms.
> > > unsigned long is too short when sh_offset is Elf64_Off which
> > > would be the case on 64bit ELF headers.
> > >
> > > This problem was found while adding an error message to print
> > > sh_offset and sh_size. If sh_offset + sh_size exceed the size
> > > of the local variable, the checks for overflow and offset/size
> > > being too large will not find the problem and call the section
> > > offset valid. This failure might cause problems later on.
> > >
> > > Fix the overflow problem using the right size local variable when
> > > CONFIG_64BIT is defined.
> > >
> > > Signed-off-by: Shuah Khan <[email protected]>
> > > ---
> > > Changes since v1:
> > > - Updated commit log to describe the fix clearly. No code
> > > changes.
> >
> > Thanks! But the implications of your fix is beyond what is described.
> > Although not a real issue today in practice.
> >
> > I think we should extend it with something like this, let me know
> > what you think (I can just ammend the commit log, no resend would
> > be needed):
> >
> > Without this fix applied we were shorting the design of modules to
> > have section headers placed within the 32-bit boundary (4 GiB) instead of
> > 64-bits when on 64-bit architectures (which allows for up to 16,777,216
> > TiB). In practice this just meant we were limiting modules to below
> > 4 GiB even on 64-bit systems. This then should not really affect any
> > real-world use case as modules these days obviously should likely never
> > exceed 1 GiB in size. A specially crafted invalid module might succeed to
> > skip validation in validate_section_offset() due to this mistake, but in such
> > case no impact is observed through code inspection given the correct data
> > types are used for the copy of the module when needed on move_module() when
> > the section type is not SHT_NOBITS (which indicates no the section
> > occupies no space on the file).
> >
>
> Sounds good to me.
OK pushed with the change above added. Thanks!
Luis