Re: [PATCH] pstore: Remove bogus format string definition

[Kees Cook]

On Wed, May 30, 2018 at 1:30 PM, Arnd Bergmann <arnd@xxxxxxxx> wrote:
> The pstore conversion to timespec64 introduces its own method of passing
> seconds into sscanf() and sprintf() type functions to work around the
> timespec64 definition on 64-bit systems that redefine it to 'timespec'.
>
> That hack is now finally getting removed, but that means we get a (harmless)
> warning once both patches are merged:
>
> fs/pstore/ram.c: In function 'ramoops_read_kmsg_hdr':
> fs/pstore/ram.c:39:29: error: format '%ld' expects argument of type 'long int *', but argument 3 has type 'time64_t *' {aka 'long long int *'} [-Werror=format=]
>  #define RAMOOPS_KERNMSG_HDR "===="
>                              ^~~~~~
> fs/pstore/ram.c:167:21: note: in expansion of macro 'RAMOOPS_KERNMSG_HDR'
>
> This removes the pstore specific workaround and uses the same method that
> we have in place for all other functions that print a timespec64.
>
> Related to this, I found that the kasprintf() output contains an incorrect
> nanosecond values for any number starting with zeroes, and I adapt the
> format string accordingly.
>
> Link: https://lkml.org/lkml/2018/5/19/115
> Link: https://lkml.org/lkml/2018/5/16/1080
> Fixes: 0f0d83b99ef7 ("pstore: Convert internal records to timespec64")
> Signed-off-by: Arnd Bergmann <arnd@xxxxxxxx>

Acked-by: Kees Cook <keescook@xxxxxxxxxxxx>

Please feel free to carry this patch in which ever series/tree it is needed. :P

-Kees

> ---
>  fs/pstore/ram.c | 17 ++++++-----------
>  1 file changed, 6 insertions(+), 11 deletions(-)
>
> diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
> index 69e893076ab7..bbd1e357c23d 100644
> --- a/fs/pstore/ram.c
> +++ b/fs/pstore/ram.c
> @@ -38,11 +38,6 @@
>
>  #define RAMOOPS_KERNMSG_HDR "===="
>  #define MIN_MEM_SIZE 4096UL
> -#if __BITS_PER_LONG == 64
> -# define TVSEC_FMT "%ld"
> -#else
> -# define TVSEC_FMT "%lld"
> -#endif
>
>  static ulong record_size = MIN_MEM_SIZE;
>  module_param(record_size, ulong, 0400);
> @@ -164,15 +159,15 @@ static int ramoops_read_kmsg_hdr(char *buffer, struct timespec64 *time,
>         char data_type;
>         int header_length = 0;
>
> -       if (sscanf(buffer, RAMOOPS_KERNMSG_HDR TVSEC_FMT ".%lu-%c\n%n",
> -                  &time->tv_sec, &time->tv_nsec, &data_type,
> +       if (sscanf(buffer, RAMOOPS_KERNMSG_HDR "%lld.%lu-%c\n%n",
> +                  (time64_t *)&time->tv_sec, &time->tv_nsec, &data_type,
>                    &header_length) == 3) {
>                 if (data_type == 'C')
>                         *compressed = true;
>                 else
>                         *compressed = false;
> -       } else if (sscanf(buffer, RAMOOPS_KERNMSG_HDR TVSEC_FMT ".%lu\n%n",
> -                         &time->tv_sec, &time->tv_nsec,
> +       } else if (sscanf(buffer, RAMOOPS_KERNMSG_HDR "%lld.%lu\n%n",
> +                         (time64_t *)&time->tv_sec, &time->tv_nsec,
>                           &header_length) == 2) {
>                 *compressed = false;
>         } else {
> @@ -367,8 +362,8 @@ static size_t ramoops_write_kmsg_hdr(struct persistent_ram_zone *prz,
>         char *hdr;
>         size_t len;
>
> -       hdr = kasprintf(GFP_ATOMIC, RAMOOPS_KERNMSG_HDR TVSEC_FMT ".%lu-%c\n",
> -               record->time.tv_sec,
> +       hdr = kasprintf(GFP_ATOMIC, RAMOOPS_KERNMSG_HDR "%lld.%06lu-%c\n",
> +               (time64_t)record->time.tv_sec,
>                 record->time.tv_nsec / 1000,
>                 record->compressed ? 'C' : 'D');
>         WARN_ON_ONCE(!hdr);
> --
> 2.9.0
>



-- 
Kees Cook
Pixel Security