Re: [PATCH v3 2/2] hexdump: add a new dump prefix DUMP_PREFIX_CUSTOM

[Leizhen (ThunderTown)]

On 2023/8/15 22:30, Petr Mladek wrote:
> Added Kees and hardening mailing list into Cc.
> 
> On Fri 2023-08-11 15:49:21, thunder.leizhen@xxxxxxxxxxxxxxx wrote:
>> From: Zhen Lei <thunder.leizhen@xxxxxxxxxx>
>>
>> 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.

Yes, it has these problems. So, back to v2, how about add DUMP_PREFIX_ADDRESS_LOW16?
Or named DUMP_PREFIX_ADDR16 or others. Or change the format of DUMP_PREFIX_ADDRESS
from "%p" to "%px"，Or add DUMP_PREFIX_RAWADDR. Or keep the status quo.

Also, do you have any comments on patch 1/2?

> 
> 
> 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@xxxxxxxxxx>
>> ---
>>  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
> .
> 

-- 
Regards,
  Zhen Lei