On Wed, Nov 16, 2022 at 04:35:14PM +0000, Kevin Brodsky wrote:
On 15/11/2022 13:00, Beata Michalska wrote:
Introduce new printk's pointer format extension to support capabilities, with one being denoted by specifying the 'l' length modifier alongside the 'p' type character => "%lp". In addition to the raw capability format, adding '#' flag will generate the simplified one, being slightly more verbose. In both cases, the actual result is subject to pointer hashing (address only) unless either 'x' qualifier or no_hash_pointers command line parameter is being provided. For more details see the docs, updated here as well.
While at it, add some testcases to validate new format alongside checkpatchpl new warning, to raise the awareness of potential misuse.
Signed-off-by: Beata Michalska beata.michalska@arm.com
[...]
--- a/Documentation/core-api/printk-formats.rst +++ b/Documentation/core-api/printk-formats.rst @@ -202,6 +202,71 @@ Example:: printk("test: difference between pointers: %td\n", ptr2 - ptr1); +Capabilities +------------
+::
- %lp[x] 1:ffffc00000010005:0123456789abcdef
- %#lp[x] 0x0123456789abcdef [rwxRWE,0x0000000000000000-0xffffffffffffffff]
Nit: quite a few double empty lines in the file now, one empty line should be enough.
To be fixed in v4.
+For printing CHERI architectural capabilities (when supported, otherwise +the result of using it is undefined). Formatting based on: +https://github.com/CTSRD-CHERI/cheri-c-programming/wiki/Displaying-Capabilit... +with a small detour of dropping support for basic format in favour of the raw +one, with the assumption that the actual address can be printed with existing +printk formats (subject to appropriate casting when dealing with capabilities +and non-capability formats, which otherwise renders undefined behaviour).
+The ``l`` modifier distinguishes capability pointers from standard (integer) +pointers, giving the following capability format being printed::
- Capability tag:High bits:Low bits
<tag>:<127:64>:<63:0>+Specifying ``#`` modifier results in printing capabilities in a simplified format::
<address> [<permissions>,<base>-<top>] (<attr>)
+where::
- permissions - none or any combination of:
- 'r' : CHERI_PERM_LOAD- 'w' : CHERI_PERM_STORE- 'x' : CHERI_PERM_EXECUTE- 'R' : CHERI_PERM_LOAD_CAP- 'W' : CHERI_PERM_STORE_CAP- 'E' : ARM_CAP_PERMISSION_EXECUTIVE
- base - lower bound
- top - upper bound
- attr - none or any combination of:
- 'invalid' : capability's tag is clear- 'sentry' : capability is a sealed entry- 'sealed' : capability is sealed with a type other than thesealed entry object type+The above applies to formats with either ``no_hash_pointers`` parameter being +enabled or ``x`` modifier being provided - otherwise subject to hashing +(address only, with remaining bits being disregarded)
Nit: full stop.
Ditto
[...]
- /*
* Basic checks so that the actual output can be safely compared* to the expected one*/- if (!cheri_tag_get(cap) || cheri_perms_get(cap) != perms
|| cheri_is_sealed(cap)) {pr_warn("Failed to create capability for testing - skipping");skipped_tests += no_hash_pointers ?ARRAY_SIZE(cap_fmt_non_hashed) + ARRAY_SIZE(setup) :ARRAY_SIZE(cap_fmt_non_hashed) + ARRAY_SIZE(setup)/2;Alright I see better what's happening now, but I think there are still two issues:
- You're not updating total_tests in the cap_fmt_non_hashed loop, also
it would probably make sense not to abort on failure (just increase failed_tests), like in __test().
Right, I have missed that as plain (%p test) is not doing that.
- I think you have either one or two tests per iteration of the setup
array, so ARRAY_SIZE(setup) * 2 or ARRAY_SIZE(setup).
Obviously - thanks for catching that!
return;- }
[...]
+non_hashed:
- for (int i = 0; i < ARRAY_SIZE(setup); ++i) {
void * __capability current_cap;switch (setup[i].action) {case CAP_TAG_CLEAR:current_cap = cheri_tag_clear(cap);break;case CAP_NULL_DERIVE:/* Null-derived capability */current_cap = cheri_address_set(0,cheri_address_get(cap));Nit: the line break looks a bit strange, I think in that case being 2 char over 80 is a better trade-off in readability.
Done
break;default:current_cap = cap;break;}
[...]
@@ -3138,7 +3333,28 @@ int vbin_printf(u32 *bin_buf, size_t size, const char *fmt, va_list args) while (isalnum(*fmt)) fmt++; break; +#ifdef __CHERI__
case FORMAT_TYPE_CAPABILITY:/** Capabilities shall be handled now: subject to* required alignment, plus exercising extra caution* when storing the capability in the buffer directly.* Otherwise breaking down the capability* to extract the information requested (format)* would be rather counter-productive.I'm still not 100% sure what the last sentence refers to, is it that we could have just saved the capability if not for the two reasons above?
I agree it's not clearly laid out. An alternative to handling capabilities here (as per formatting), or storing it in the buffer, was to break it into tag, metadata and address values, and store those in the buffer instead. Dropping that line in v4.
Kevin
*/str = capability(fmt, str, end,va_arg(args, void * __capability),spec);if (str + 1 < end)*str++ = '\0';elseend[-1] = '\0'; /* Must be null terminated */while (isalnum(*fmt))fmt++;
break;+#endif default: switch (spec.type) { @@ -3319,7 +3535,22 @@ int bstr_printf(char *buf, size_t size, const char *fmt, const u32 *bin_buf) fmt++; break; } +#ifdef __CHERI__
case FORMAT_TYPE_CAPABILITY: {long length = strlen(args);if (str < end) {memcpy(str, args, min(length, (end - str)));I think you still have the second issue I mentioned in v2, i.e. the length you use in the two lines below should be the same length memcpy() is passed here, like in FORMAT_TYPE_PTR.
What FORMAT_TYPE_PTR is doing is the same already:
FORMAT_TYPE_PTR FORMAT_TYPE_CAPABILITY ---------------------------------------------- str += len; -> str += length; args += len + 1; -> args += length + 1;
where: len = copy = strlen(args); -> length = strlen(args);
and with:
memcpy(str, args, copy); -> memcpy(str, args, min(length, (end - str)));
The only difference between the two being:
/* copy = strlen(args); */ if (copy > end - str) copy = end - str; => /* copy == min(length, (end - str)) */ memcpy(str, args, copy); memcpy(str, args, min(length, (end - str)));
Note that the 'str' will be updated based on the expected, not actual length. As per doc comments for bstr_printf:
* The return value is the number of characters which would * be generated for the given input ....
Or am I still missing smth (which might be the case)?
--- BR B.
str += length;args += length + 1;}
while (isalnum(*fmt))fmt++;break;}+#endif case FORMAT_TYPE_PERCENT_CHAR: if (str < end) *str = '%'; diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl index 577e02998701..867e0a01ae9f 100755 --- a/scripts/checkpatch.pl +++ b/scripts/checkpatch.pl @@ -6760,7 +6760,7 @@ sub process { my $fmt = get_quoted_string($lines[$count - 1], raw_line($count, 0)); $fmt =~ s/%%//g;
while ($fmt =~ /(\%[\*\d\.]*p(\w)(\w*))/g) {
while ($fmt =~ /(\%[\*\d\.#l]*p(\w)(\w*))/g) { $specifier = $1; $extension = $2; $qualifier = $3;@@ -6768,7 +6768,8 @@ sub process { ($extension eq "f" && defined $qualifier && $qualifier !~ /^w/) || ($extension eq "4" &&
defined $qualifier && $qualifier !~ /^cc/)) {
defined $qualifier && $qualifier !~ /^cc/) ||($specifier =~ /lp/ && $extension ne "x")) { $bad_specifier = $specifier; last; }@@ -6780,6 +6781,14 @@ sub process { "Using vsprintf specifier '%px' potentially exposes the kernel memory layout, if you don't really need the address please consider using '%p'.\n" . "$here\n$stat_real\n"); } }
if ($fmt =~ /\%[\*\d\.#]*lp/) {my $stat_real = get_stat_real($linenr, $lc);WARN("VSPRINTF_POINTER_EXTENSION","Using vsprintf specifier '\%[#]lp[x]' is undefined behaviour when used with non-capability types. Make sure you got it right.\n" . "$here\n$stat_real\n");}if ($bad_specifier ne "") { my $stat_real = get_stat_real($linenr, $lc); my $ext_type = "Invalid";