Re: [PATCH v2] efi_loader: Fix memory corruption on 32bit systems
Am 28. Juli 2023 03:51:55 MESZ schrieb Simon Glass :
>Hi,
>
>On Thu, 27 Jul 2023 at 08:36, Dan Carpenter wrote:
>>
>> On Thu, Jul 27, 2023 at 11:22:15AM +0300, Ilias Apalodimas wrote:
>> > Hi Dan,
>> >
>> > [...]
>> >
>> > > @@ -313,7 +313,7 @@ static int cmp_pe_section(const void *arg1, const
>> > > void *arg2)
>> > > *
>> > > * Return: valid pointer to a image, return NULL if allocation fails.
>> > > */
>> > > -void *efi_prepare_aligned_image(void *efi, u64 *efi_size)
>> > > +void *efi_prepare_aligned_image(void *efi, size_t *efi_size)
>> > > {
>> > > size_t new_efi_size;
>> > > void *new_efi;
>> > > @@ -600,7 +600,7 @@ static bool efi_image_authenticate(void *efi, size_t
>> > > efi_size)
>> > > if (!efi_secure_boot_enabled())
>> > > return true;
>> > >
>> > > - new_efi = efi_prepare_aligned_image(efi, (u64 *)_size);
>> > > + new_efi = efi_prepare_aligned_image(efi, _size);
>> > > if (!new_efi)
>> > > return false;
>> > >
>> > > diff --git a/lib/efi_loader/efi_tcg2.c b/lib/efi_loader/efi_tcg2.c
>> > > index 49f8a5e77cbf..d57afd0c498b 100644
>> > > --- a/lib/efi_loader/efi_tcg2.c
>> > > +++ b/lib/efi_loader/efi_tcg2.c
>> > > @@ -882,7 +882,7 @@ out:
>> > > *
>> > > * Return: status code
>> > > */
>> > > -static efi_status_t tcg2_hash_pe_image(void *efi, u64 efi_size,
>> > > +static efi_status_t tcg2_hash_pe_image(void *efi, size_t efi_size,
>> > >struct tpml_digest_values
>> > > *digest_list)
>> >
>> > Unfortunately the rabbit hole is a bit deeper with this one.
>> > tcg2_hash_pe_image() is called in
>> > - tcg2_measure_pe_image(). This one is called in efi_load_pe() and the type
>> > is indeed a size_t there, so that's fine
>> > - efi_tcg2_hash_log_extend_event(), this one is different...
>> > The function is described by the EFI spec [0] which mandates a u64... I
>> > think that was the reason efi_prepare_aligned_image() is using a u64 to
>> > begin with. This one uses the size only though not the pointer, but in a
>> > 32bit platform it would truncate s size > UINT_MAX.
>> >
>> > [0]
>> > https://trustedcomputinggroup.org/wp-content/uploads/EFI-Protocol-Specification-rev13-160330final.pdf
>>
>> I have maybe misread something... I don't think this is a real issue.
>> 32bit systems aren't going to be able to allocate that much memory
>> anyway. Also there are a lot of size_t parameters already so it's not
>> a new issue.
>
>We should really use ulong for addresses and malloc() sizes.
Please, do not abuse long.
According to the C specification the size of long is independent of the size of
pointers.
Addresses should be pointers and sizes should be size_t.
Best regards
Heinrich
Re: [PATCH v2] efi_loader: Fix memory corruption on 32bit systems
Hi,
On Thu, 27 Jul 2023 at 08:36, Dan Carpenter wrote:
>
> On Thu, Jul 27, 2023 at 11:22:15AM +0300, Ilias Apalodimas wrote:
> > Hi Dan,
> >
> > [...]
> >
> > > @@ -313,7 +313,7 @@ static int cmp_pe_section(const void *arg1, const
> > > void *arg2)
> > > *
> > > * Return: valid pointer to a image, return NULL if allocation fails.
> > > */
> > > -void *efi_prepare_aligned_image(void *efi, u64 *efi_size)
> > > +void *efi_prepare_aligned_image(void *efi, size_t *efi_size)
> > > {
> > > size_t new_efi_size;
> > > void *new_efi;
> > > @@ -600,7 +600,7 @@ static bool efi_image_authenticate(void *efi, size_t
> > > efi_size)
> > > if (!efi_secure_boot_enabled())
> > > return true;
> > >
> > > - new_efi = efi_prepare_aligned_image(efi, (u64 *)_size);
> > > + new_efi = efi_prepare_aligned_image(efi, _size);
> > > if (!new_efi)
> > > return false;
> > >
> > > diff --git a/lib/efi_loader/efi_tcg2.c b/lib/efi_loader/efi_tcg2.c
> > > index 49f8a5e77cbf..d57afd0c498b 100644
> > > --- a/lib/efi_loader/efi_tcg2.c
> > > +++ b/lib/efi_loader/efi_tcg2.c
> > > @@ -882,7 +882,7 @@ out:
> > > *
> > > * Return: status code
> > > */
> > > -static efi_status_t tcg2_hash_pe_image(void *efi, u64 efi_size,
> > > +static efi_status_t tcg2_hash_pe_image(void *efi, size_t efi_size,
> > >struct tpml_digest_values *digest_list)
> >
> > Unfortunately the rabbit hole is a bit deeper with this one.
> > tcg2_hash_pe_image() is called in
> > - tcg2_measure_pe_image(). This one is called in efi_load_pe() and the type
> > is indeed a size_t there, so that's fine
> > - efi_tcg2_hash_log_extend_event(), this one is different...
> > The function is described by the EFI spec [0] which mandates a u64... I
> > think that was the reason efi_prepare_aligned_image() is using a u64 to
> > begin with. This one uses the size only though not the pointer, but in a
> > 32bit platform it would truncate s size > UINT_MAX.
> >
> > [0]
> > https://trustedcomputinggroup.org/wp-content/uploads/EFI-Protocol-Specification-rev13-160330final.pdf
>
> I have maybe misread something... I don't think this is a real issue.
> 32bit systems aren't going to be able to allocate that much memory
> anyway. Also there are a lot of size_t parameters already so it's not
> a new issue.
We should really use ulong for addresses and malloc() sizes.
Regards,
Simon
Re: [PATCH v2] efi_loader: Fix memory corruption on 32bit systems
On Thu, Jul 27, 2023 at 11:22:15AM +0300, Ilias Apalodimas wrote:
> Hi Dan,
>
> [...]
>
> > @@ -313,7 +313,7 @@ static int cmp_pe_section(const void *arg1, const void
> > *arg2)
> > *
> > * Return: valid pointer to a image, return NULL if allocation fails.
> > */
> > -void *efi_prepare_aligned_image(void *efi, u64 *efi_size)
> > +void *efi_prepare_aligned_image(void *efi, size_t *efi_size)
> > {
> > size_t new_efi_size;
> > void *new_efi;
> > @@ -600,7 +600,7 @@ static bool efi_image_authenticate(void *efi, size_t
> > efi_size)
> > if (!efi_secure_boot_enabled())
> > return true;
> >
> > - new_efi = efi_prepare_aligned_image(efi, (u64 *)_size);
> > + new_efi = efi_prepare_aligned_image(efi, _size);
> > if (!new_efi)
> > return false;
> >
> > diff --git a/lib/efi_loader/efi_tcg2.c b/lib/efi_loader/efi_tcg2.c
> > index 49f8a5e77cbf..d57afd0c498b 100644
> > --- a/lib/efi_loader/efi_tcg2.c
> > +++ b/lib/efi_loader/efi_tcg2.c
> > @@ -882,7 +882,7 @@ out:
> > *
> > * Return: status code
> > */
> > -static efi_status_t tcg2_hash_pe_image(void *efi, u64 efi_size,
> > +static efi_status_t tcg2_hash_pe_image(void *efi, size_t efi_size,
> >struct tpml_digest_values *digest_list)
>
> Unfortunately the rabbit hole is a bit deeper with this one.
> tcg2_hash_pe_image() is called in
> - tcg2_measure_pe_image(). This one is called in efi_load_pe() and the type
> is indeed a size_t there, so that's fine
> - efi_tcg2_hash_log_extend_event(), this one is different...
> The function is described by the EFI spec [0] which mandates a u64... I
> think that was the reason efi_prepare_aligned_image() is using a u64 to
> begin with. This one uses the size only though not the pointer, but in a
> 32bit platform it would truncate s size > UINT_MAX.
>
> [0]
> https://trustedcomputinggroup.org/wp-content/uploads/EFI-Protocol-Specification-rev13-160330final.pdf
I have maybe misread something... I don't think this is a real issue.
32bit systems aren't going to be able to allocate that much memory
anyway. Also there are a lot of size_t parameters already so it's not
a new issue.
regards,
dan carpenter
[PATCH v2] efi_loader: Fix memory corruption on 32bit systems
The issue is this line:
new_efi = efi_prepare_aligned_image(efi, (u64 *)_size);
The efi_size variable is type size_t and on a 32 bit system that's 32
bits. The u64 type is obviously 64 bits. So we write 8 bytes to a 4
byte buffer which corrupts memory.
Fix this by changing the type of efi_prepare_aligned_image() to a
size_t pointer.
Signed-off-by: Dan Carpenter
---
v2: Change efi_prepare_aligned_image() instead of changing
efi_image_authenticate(). This is a cleaner way to fix the problem.
include/efi_loader.h | 2 +-
lib/efi_loader/efi_image_loader.c | 4 ++--
lib/efi_loader/efi_tcg2.c | 2 +-
3 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/include/efi_loader.h b/include/efi_loader.h
index b5fa0fe01ded..9c1a9ed16af6 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -1022,7 +1022,7 @@ bool efi_secure_boot_enabled(void);
bool efi_capsule_auth_enabled(void);
-void *efi_prepare_aligned_image(void *efi, u64 *efi_size);
+void *efi_prepare_aligned_image(void *efi, size_t *efi_size);
bool efi_image_parse(void *efi, size_t len, struct efi_image_regions **regp,
WIN_CERTIFICATE **auth, size_t *auth_len);
diff --git a/lib/efi_loader/efi_image_loader.c
b/lib/efi_loader/efi_image_loader.c
index 26df0da16c93..64980008403b 100644
--- a/lib/efi_loader/efi_image_loader.c
+++ b/lib/efi_loader/efi_image_loader.c
@@ -313,7 +313,7 @@ static int cmp_pe_section(const void *arg1, const void
*arg2)
*
* Return: valid pointer to a image, return NULL if allocation fails.
*/
-void *efi_prepare_aligned_image(void *efi, u64 *efi_size)
+void *efi_prepare_aligned_image(void *efi, size_t *efi_size)
{
size_t new_efi_size;
void *new_efi;
@@ -600,7 +600,7 @@ static bool efi_image_authenticate(void *efi, size_t
efi_size)
if (!efi_secure_boot_enabled())
return true;
- new_efi = efi_prepare_aligned_image(efi, (u64 *)_size);
+ new_efi = efi_prepare_aligned_image(efi, _size);
if (!new_efi)
return false;
diff --git a/lib/efi_loader/efi_tcg2.c b/lib/efi_loader/efi_tcg2.c
index 49f8a5e77cbf..d57afd0c498b 100644
--- a/lib/efi_loader/efi_tcg2.c
+++ b/lib/efi_loader/efi_tcg2.c
@@ -882,7 +882,7 @@ out:
*
* Return: status code
*/
-static efi_status_t tcg2_hash_pe_image(void *efi, u64 efi_size,
+static efi_status_t tcg2_hash_pe_image(void *efi, size_t efi_size,
struct tpml_digest_values *digest_list)
{
WIN_CERTIFICATE *wincerts = NULL;
--
2.39.2
Re: [PATCH v2] efi_loader: Fix memory corruption on 32bit systems
Hi Dan,
[...]
> @@ -313,7 +313,7 @@ static int cmp_pe_section(const void *arg1, const void
> *arg2)
> *
> * Return: valid pointer to a image, return NULL if allocation fails.
> */
> -void *efi_prepare_aligned_image(void *efi, u64 *efi_size)
> +void *efi_prepare_aligned_image(void *efi, size_t *efi_size)
> {
> size_t new_efi_size;
> void *new_efi;
> @@ -600,7 +600,7 @@ static bool efi_image_authenticate(void *efi, size_t
> efi_size)
> if (!efi_secure_boot_enabled())
> return true;
>
> - new_efi = efi_prepare_aligned_image(efi, (u64 *)_size);
> + new_efi = efi_prepare_aligned_image(efi, _size);
> if (!new_efi)
> return false;
>
> diff --git a/lib/efi_loader/efi_tcg2.c b/lib/efi_loader/efi_tcg2.c
> index 49f8a5e77cbf..d57afd0c498b 100644
> --- a/lib/efi_loader/efi_tcg2.c
> +++ b/lib/efi_loader/efi_tcg2.c
> @@ -882,7 +882,7 @@ out:
> *
> * Return: status code
> */
> -static efi_status_t tcg2_hash_pe_image(void *efi, u64 efi_size,
> +static efi_status_t tcg2_hash_pe_image(void *efi, size_t efi_size,
> struct tpml_digest_values *digest_list)
Unfortunately the rabbit hole is a bit deeper with this one.
tcg2_hash_pe_image() is called in
- tcg2_measure_pe_image(). This one is called in efi_load_pe() and the type
is indeed a size_t there, so that's fine
- efi_tcg2_hash_log_extend_event(), this one is different...
The function is described by the EFI spec [0] which mandates a u64... I
think that was the reason efi_prepare_aligned_image() is using a u64 to
begin with. This one uses the size only though not the pointer, but in a
32bit platform it would truncate s size > UINT_MAX.
[0]
https://trustedcomputinggroup.org/wp-content/uploads/EFI-Protocol-Specification-rev13-160330final.pdf
Regards
/Ilias
> {
> WIN_CERTIFICATE *wincerts = NULL;
> --
> 2.39.2
>
