Date:   Tue, 15 Aug 2023 16:30:51 +0200
From:   Petr Mladek <pmladek@...e.com>
To:     thunder.leizhen@...weicloud.com
Cc:     Sergey Senozhatsky <senozhatsky@...omium.org>,
        Steven Rostedt <rostedt@...dmis.org>,
        John Ogness <john.ogness@...utronix.de>,
        linux-kernel@...r.kernel.org,
        Zhen Lei <thunder.leizhen@...wei.com>,
        Randy Dunlap <rdunlap@...radead.org>,
        Kees Cook <keescook@...omium.org>,
        linux-hardening@...r.kernel.org
Subject: Re: [PATCH v3 2/2] hexdump: add a new dump prefix DUMP_PREFIX_CUSTOM

Added Kees and hardening mailing list into Cc.

On Fri 2023-08-11 15:49:21, thunder.leizhen@...weicloud.com wrote:
> From: Zhen Lei <thunder.leizhen@...wei.com>
> 
> Currently, function print_hex_dump() supports three dump prefixes:
> DUMP_PREFIX_NONE, DUMP_PREFIX_ADDRESS and DUMP_PREFIX_OFFSET. But for some
> usage scenarios, they don't work perfectly. For example, dump the content
> of one task's stack. In order to quickly identify a stack frame,
> DUMP_PREFIX_ADDRESS is preferred. But without boot option no_hash_pointers
> , DUMP_PREFIX_ADDRESS just print the 32-bit hash value.
> 
> dump memory at sp=ffff800080903aa0:
> 00000000a00a1d32: 80903ac0 ffff8000 8feeae24 ffffc356
> 000000007993ef27: 9811c000 ffff0d98 8ad2e500 ffff0d98
> 00000000b1a0b2de: 80903b30 ffff8000 8ff3a618 ffffc356
> ... ...
> 00000000a7a9048b: 9810b3c0 ffff0d98 00000000 00000000
> 0000000011cda415: 80903cb0 ffff8000 00000000 00000000
> 000000002dbdf9cd: 981f8400 ffff0d98 00000001 00000000
> 
> On the other hand, printing multiple 64-bit addresses is redundant when
> the 'sp' value is already printed. Generally, we do not dump more than
> 64 KiB memory. It is sufficient to print only the lower 16 bits of the
> address.
> 
> dump memory at sp=ffff800080883a90:
> 3a90: 80883ac0 ffff8000 3d8e936c ffffbd5b
> 3aa0: 5833f000 ffff3580 00000001 00000000
> 3ab0: 40299840 ffff3580 590dfa00 ffff3580
> 3ac0: 80883b30 ffff8000 3d938b28 ffffbd5b
> 3ad0: 40877180 ffff3580 590dfa00 ffff3580
> 3ae0: 4090f600 ffff3580 80883cb0 ffff8000
> 3af0: 00000010 00000000 00000000 00000000
> 3b00: 4090f700 ffff3580 00000001 00000000
> 
> Let's add DUMP_PREFIX_CUSTOM, allows users to make some adjustments to
> their needs.
> 
> For example:
> pr_info("dump memory at sp=%px:\n", sp);
> print_hex_dump(KERN_INFO, "%s%16hx: %s\n",
>                DUMP_PREFIX_CUSTOM, 16, 1, sp, 16, false);
> print_hex_dump(KERN_INFO, "%s%16x: %s\n",
>                DUMP_PREFIX_CUSTOM, 16, 1, sp, 16, false);
> print_hex_dump(KERN_INFO, "%s%px: %s\n",
>                DUMP_PREFIX_CUSTOM, 16, 1, sp, 16, false);

IMHO, this is pretty bad interface.

  + From the user POV:

    It is far from clear what values will be passed for the given
    printf format. It can be docummented but...


  + From the security POV:

    The compiler could not check if the printk() parameters
    match the format. I mean if the number of types of
    the parameters are correct.


Best Regards,
Petr

> dump memory at sp=ffff80008091baa0:
>             baa0: c0 ba 91 80 00 80 ff ff d4 38 16 ce fc a7 ff ff
>         8091baa0: c0 ba 91 80 00 80 ff ff d4 38 16 ce fc a7 ff ff
> ffff80008091baa0: c0 ba 91 80 00 80 ff ff d4 38 16 ce fc a7 ff ff
> 
> Signed-off-by: Zhen Lei <thunder.leizhen@...wei.com>
> ---
>  include/linux/printk.h |  3 ++-
>  lib/hexdump.c          | 16 ++++++++++++++++
>  2 files changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/printk.h b/include/linux/printk.h
> index 8ef499ab3c1ed2e..23779dcc4836414 100644
> --- a/include/linux/printk.h
> +++ b/include/linux/printk.h
> @@ -704,7 +704,8 @@ extern const struct file_operations kmsg_fops;
>  enum {
>  	DUMP_PREFIX_NONE,
>  	DUMP_PREFIX_ADDRESS,
> -	DUMP_PREFIX_OFFSET
> +	DUMP_PREFIX_OFFSET,
> +	DUMP_PREFIX_CUSTOM
>  };
>  extern int hex_dump_to_buffer(const void *buf, size_t len, int rowsize,
>  			      int groupsize, char *linebuf, size_t linebuflen,
> diff --git a/lib/hexdump.c b/lib/hexdump.c
> index 1064706d57c15ed..fa4a44543a946b8 100644
> --- a/lib/hexdump.c
> +++ b/lib/hexdump.c
> @@ -232,6 +232,11 @@ EXPORT_SYMBOL(hex_dump_to_buffer);
>   * @level: kernel log level (e.g. KERN_DEBUG)
>   * @prefix_str: string to prefix each line with;
>   *  caller supplies trailing spaces for alignment if desired
> + *  OR
> + *  the custom format string of DUMP_PREFIX_CUSTOM;
> + *  Corresponding to three parameters in fixed order:
> + *  <string: level> <pointer: address> <string: converted data>
> + *  For example: "%s%04hx: %s\n", "%s%.8x: %s\n", "%s%px: %s\n"
>   * @prefix_type: controls whether prefix of an offset, address, or none
>   *  is printed (%DUMP_PREFIX_OFFSET, %DUMP_PREFIX_ADDRESS, %DUMP_PREFIX_NONE)
>   * @rowsize: number of bytes to print per line; must be 16 or 32
> @@ -257,6 +262,14 @@ EXPORT_SYMBOL(hex_dump_to_buffer);
>   * 0009ab42: 40 41 42 43 44 45 46 47 48 49 4a 4b 4c 4d 4e 4f  @ABCDEFGHIJKLMNO
>   * Example output using %DUMP_PREFIX_ADDRESS and 4-byte mode:
>   * ffffffff88089af0: 73727170 77767574 7b7a7978 7f7e7d7c  pqrstuvwxyz{|}~.
> + *
> + * E.g.:
> + *   print_hex_dump(KERN_DEBUG, "%s%04hx: %s\n", DUMP_PREFIX_CUSTOM,
> + *		    16, 1, frame->data, frame->len, false);
> + *   %04hx --> Only the lower 16 bits of the address are printed.
> + *
> + * Example output using %DUMP_PREFIX_CUSTOM and 1-byte mode:
> + * 3aa0: c0 3a 8d 80 00 80 ff ff d4 38 16 1d 94 a6 ff ff
>   */
>  void print_hex_dump(const char *level, const char *prefix_str, int prefix_type,
>  		    int rowsize, int groupsize,
> @@ -292,6 +305,9 @@ void print_hex_dump(const char *level, const char *prefix_str, int prefix_type,
>  			}
>  			printk("%s%s%0*x: %s\n", level, prefix_str, width, i, linebuf);
>  			break;
> +		case DUMP_PREFIX_CUSTOM:
> +			printk(prefix_str, level, ptr + i, linebuf);
> +			break;
>  		default:
>  			printk("%s%s%s\n", level, prefix_str, linebuf);
>  			break;
> -- 
> 2.34.1

Powered by blists - more mailing lists