[PATCH] pstore: Fix an overflow on 32-bit builds.

[PATCH] pstore: Fix an overflow on 32-bit builds.

[Andrzej Zaborowski]

[resend]
In generic_id the long int timestamp is multiplied by 100000 and needs
an explicit cast to u64.

Without that the id in the resulting pstore filename is wrong and
userspace may have problems parsing it, but more importantly files in
pstore can never be deleted and may fill the EFI flash (brick device?).
This happens because when generic pstore code wants to delete a file,
it passes the id to the EFI backend which reinterpretes it and a wrong
variable name is attempted to be deleted.  There's no error message but
after remounting pstore, deleted files would reappear.

Signed-off-by: Andrew Zaborowski <andrew.zaborowski@intel.com>
---
 drivers/firmware/efi/efi-pstore.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/firmware/efi/efi-pstore.c b/drivers/firmware/efi/efi-pstore.c
index 4b9dc83..e992abc 100644
--- a/drivers/firmware/efi/efi-pstore.c
+++ b/drivers/firmware/efi/efi-pstore.c
@@ -40,7 +40,7 @@ struct pstore_read_data {
 static inline u64 generic_id(unsigned long timestamp,
 			     unsigned int part, int count)
 {
-	return (timestamp * 100 + part) * 1000 + count;
+	return ((u64) timestamp * 100 + part) * 1000 + count;
 }
 
 static int efi_pstore_read_func(struct efivar_entry *entry, void *data)
-- 
1.7.7.6

Re: [PATCH] pstore: Fix an overflow on 32-bit builds.

[David Rientjes]

On Mon, 9 Jun 2014, Andrzej Zaborowski wrote:

> [resend]
> In generic_id the long int timestamp is multiplied by 100000 and needs
> an explicit cast to u64.
> 
> Without that the id in the resulting pstore filename is wrong and
> userspace may have problems parsing it, but more importantly files in
> pstore can never be deleted and may fill the EFI flash (brick device?).
> This happens because when generic pstore code wants to delete a file,
> it passes the id to the EFI backend which reinterpretes it and a wrong
> variable name is attempted to be deleted.  There's no error message but
> after remounting pstore, deleted files would reappear.
> 
> Signed-off-by: Andrew Zaborowski <andrew.zaborowski@intel.com>

Acked-by: David Rientjes <rientjes@google.com>

> ---
>  drivers/firmware/efi/efi-pstore.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/firmware/efi/efi-pstore.c b/drivers/firmware/efi/efi-pstore.c
> index 4b9dc83..e992abc 100644
> --- a/drivers/firmware/efi/efi-pstore.c
> +++ b/drivers/firmware/efi/efi-pstore.c
> @@ -40,7 +40,7 @@ struct pstore_read_data {
>  static inline u64 generic_id(unsigned long timestamp,
>  			     unsigned int part, int count)
>  {
> -	return (timestamp * 100 + part) * 1000 + count;
> +	return ((u64) timestamp * 100 + part) * 1000 + count;
>  }
>  
>  static int efi_pstore_read_func(struct efivar_entry *entry, void *data)

This fixes commit fdeadb43fdf1 ("efi-pstore: Make efi-pstore return a 
unique id") that went into stable, so I'm not sure if this should go into 
stable as well.

You probably had to resend this because you didn't email any of the 
maintainers (fixed).  Use scripts/get_maintainer.pl to figure out who to 
email about a patch.

Re: [PATCH] pstore: Fix an overflow on 32-bit builds.

[David Rientjes]

On Mon, 9 Jun 2014, Andrzej Zaborowski wrote:

> [resend]
> In generic_id the long int timestamp is multiplied by 100000 and needs
> an explicit cast to u64.
> 
> Without that the id in the resulting pstore filename is wrong and
> userspace may have problems parsing it, but more importantly files in
> pstore can never be deleted and may fill the EFI flash (brick device?).
> This happens because when generic pstore code wants to delete a file,
> it passes the id to the EFI backend which reinterpretes it and a wrong
> variable name is attempted to be deleted.  There's no error message but
> after remounting pstore, deleted files would reappear.
> 
> Signed-off-by: Andrew Zaborowski <andrew.zaborowski-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

Acked-by: David Rientjes <rientjes-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>

> ---
>  drivers/firmware/efi/efi-pstore.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/firmware/efi/efi-pstore.c b/drivers/firmware/efi/efi-pstore.c
> index 4b9dc83..e992abc 100644
> --- a/drivers/firmware/efi/efi-pstore.c
> +++ b/drivers/firmware/efi/efi-pstore.c
> @@ -40,7 +40,7 @@ struct pstore_read_data {
>  static inline u64 generic_id(unsigned long timestamp,
>  			     unsigned int part, int count)
>  {
> -	return (timestamp * 100 + part) * 1000 + count;
> +	return ((u64) timestamp * 100 + part) * 1000 + count;
>  }
>  
>  static int efi_pstore_read_func(struct efivar_entry *entry, void *data)

This fixes commit fdeadb43fdf1 ("efi-pstore: Make efi-pstore return a 
unique id") that went into stable, so I'm not sure if this should go into 
stable as well.

You probably had to resend this because you didn't email any of the 
maintainers (fixed).  Use scripts/get_maintainer.pl to figure out who to 
email about a patch.

Re: [PATCH] pstore: Fix an overflow on 32-bit builds.

[Matt Fleming]

On Mon, 09 Jun, at 01:24:43PM, David Rientjes wrote:
> On Mon, 9 Jun 2014, Andrzej Zaborowski wrote:
> 
> > [resend]
> > In generic_id the long int timestamp is multiplied by 100000 and needs
> > an explicit cast to u64.
> > 
> > Without that the id in the resulting pstore filename is wrong and
> > userspace may have problems parsing it, but more importantly files in
> > pstore can never be deleted and may fill the EFI flash (brick device?).
> > This happens because when generic pstore code wants to delete a file,
> > it passes the id to the EFI backend which reinterpretes it and a wrong
> > variable name is attempted to be deleted.  There's no error message but
> > after remounting pstore, deleted files would reappear.

It shouldn't be possible to brick devices because the efi-pstore code
still goes through efivar_entry_set_safe() whic has the necessary
checks. Please let me know if you've witnessed any fallout from this bug
other than being unable to delete files.

> This fixes commit fdeadb43fdf1 ("efi-pstore: Make efi-pstore return a 
> unique id") that went into stable, so I'm not sure if this should go into 
> stable as well.
 
I think it should go to stable too. Tony?

> You probably had to resend this because you didn't email any of the 
> maintainers (fixed).  Use scripts/get_maintainer.pl to figure out who to 
> email about a patch.

Thanks for triaging this David.

Unless anyone speaks up I'm going to throw this into the EFI tree with
David's Acked-by.

-- 
Matt Fleming, Intel Open Source Technology Center