%#lp results in the address being prefixed with 0x, like %#p, but not if the capability is null-derived. Make sure the prefix is used in all cases.
Fixes: ("lib/vsprintf: Add support for architectural capabilities") Signed-off-by: Kevin Brodsky kevin.brodsky@arm.com ---
This is probably the result of some confusion during the review process for the original patch, as the first version never printed the prefix.
Review branch:
https://git.morello-project.org/kbrodsky-arm/linux/-/commits/morello/printk_...
lib/vsprintf.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-)
diff --git a/lib/vsprintf.c b/lib/vsprintf.c index 0b551cf89880..1a2a01843775 100644 --- a/lib/vsprintf.c +++ b/lib/vsprintf.c @@ -2550,13 +2550,9 @@ char *capability(const char *fmt, char *buf, char *end, void * __capability cap, * Same applies when hashing is active. */ if ((!cheri_tag_get(cap) && !__builtin_cheri_copy_from_high(cap)) || - (likely(!no_hash_pointers) && *fmt != 'x')) { - - /* Avoid adding prefix */ - spec.flags &= ~SPECIAL; + (likely(!no_hash_pointers) && *fmt != 'x')) return pointer(fmt, buf, end, (void *)cheri_address_get(cap), spec); - }
if (spec.flags & SPECIAL) { /* Simplified format for capabilities */ int orig_field_width = spec.field_width;
On Thu, Feb 08, 2024 at 09:29:26AM +0000, Kevin Brodsky wrote:
%#lp results in the address being prefixed with 0x, like %#p, but not if the capability is null-derived. Make sure the prefix is used in all cases.
I think it was done on purpose as (per [1]): - "In addition, for null-derived capabilities (capabilities where the tag and upper attribute word are all zero), only the address is displayed (the Basic Format)." - Basic Format: ... "This should generally match the existing format used with integer pointers." - "Linux kernel displays integer pointers as hexadecimal without a 0x prefix), and p should match a platform's existing format."
And I think the reason why "%#p" works is just a coincidence as '#' is being supported by relevant formatting function (number) which is used when printing default integer pointers. It is not by 'design' as such.
--- [1] https://github.com/CTSRD-CHERI/cheri-c-programming/wiki/Displaying-Capabilit... --- BR Beata
Fixes: ("lib/vsprintf: Add support for architectural capabilities") Signed-off-by: Kevin Brodsky kevin.brodsky@arm.com
This is probably the result of some confusion during the review process for the original patch, as the first version never printed the prefix.
Review branch:
https://git.morello-project.org/kbrodsky-arm/linux/-/commits/morello/printk_...
lib/vsprintf.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-)
diff --git a/lib/vsprintf.c b/lib/vsprintf.c index 0b551cf89880..1a2a01843775 100644 --- a/lib/vsprintf.c +++ b/lib/vsprintf.c @@ -2550,13 +2550,9 @@ char *capability(const char *fmt, char *buf, char *end, void * __capability cap, * Same applies when hashing is active. */ if ((!cheri_tag_get(cap) && !__builtin_cheri_copy_from_high(cap)) ||
(likely(!no_hash_pointers) && *fmt != 'x')) {/* Avoid adding prefix */spec.flags &= ~SPECIAL;
(likely(!no_hash_pointers) && *fmt != 'x'))
- }
if (spec.flags & SPECIAL) { /* Simplified format for capabilities */ int orig_field_width = spec.field_width; -- 2.43.0
On 08/02/2024 13:07, Beata Michalska wrote:
On Thu, Feb 08, 2024 at 09:29:26AM +0000, Kevin Brodsky wrote:
%#lp results in the address being prefixed with 0x, like %#p, but not if the capability is null-derived. Make sure the prefix is used in all cases.
I think it was done on purpose as (per [1]):
- "In addition, for null-derived capabilities (capabilities where the tag and upper attribute word are all zero), only the address is displayed (the Basic Format)."
[1] is indeed not very clear on the format in that case, but CheriBSD does print 0x for null-derived capabilities too [2] and so does Musl. It would be strange to print it only if the capability is not null-derived.
- Basic Format: ... "This should generally match the existing format used with integer pointers."
- "Linux kernel displays integer pointers as hexadecimal without a 0x prefix), and p should match a platform's existing format."
And I think the reason why "%#p" works is just a coincidence as '#' is being supported by relevant formatting function (number) which is used when printing default integer pointers. It is not by 'design' as such.
I don't have a strong opinion either for or against printing the prefix, but we should definitely be consistent (either always or never print it).
Kevin
[2] https://github.com/CTSRD-CHERI/cheribsd/blob/main/lib/libc/stdio/printfcommo...
[1] https://github.com/CTSRD-CHERI/cheri-c-programming/wiki/Displaying-Capabilit...
BR Beata
On Fri, Feb 09, 2024 at 02:56:51PM +0000, Kevin Brodsky wrote:
On 08/02/2024 13:07, Beata Michalska wrote:
On Thu, Feb 08, 2024 at 09:29:26AM +0000, Kevin Brodsky wrote:
%#lp results in the address being prefixed with 0x, like %#p, but not if the capability is null-derived. Make sure the prefix is used in all cases.
I think it was done on purpose as (per [1]):
- "In addition, for null-derived capabilities (capabilities where the tag and upper attribute word are all zero), only the address is displayed (the Basic Format)."
[1] is indeed not very clear on the format in that case, but CheriBSD does print 0x for null-derived capabilities too [2] and so does Musl. It would be strange to print it only if the capability is not null-derived.
- Basic Format: ... "This should generally match the existing format used with integer pointers."
- "Linux kernel displays integer pointers as hexadecimal without a 0x prefix), and p should match a platform's existing format."
And I think the reason why "%#p" works is just a coincidence as '#' is being supported by relevant formatting function (number) which is used when printing default integer pointers. It is not by 'design' as such.
I don't have a strong opinion either for or against printing the prefix, but we should definitely be consistent (either always or never print it).
I think I would be more inclined to drop the prefix from the Simplified formatting - so no prefix in both cases: valid or null-derived. That is to be consistent with how Linux handles printing pointers. Though that's just my preference.
--- BR Beata
Kevin
[2] https://github.com/CTSRD-CHERI/cheribsd/blob/main/lib/libc/stdio/printfcommo...
[1] https://github.com/CTSRD-CHERI/cheri-c-programming/wiki/Displaying-Capabilit...
BR Beata
On Thu, Feb 15, 2024 at 08:14:37AM +0100, Beata Michalska wrote:
On Fri, Feb 09, 2024 at 02:56:51PM +0000, Kevin Brodsky wrote:
On 08/02/2024 13:07, Beata Michalska wrote:
On Thu, Feb 08, 2024 at 09:29:26AM +0000, Kevin Brodsky wrote:
%#lp results in the address being prefixed with 0x, like %#p, but not if the capability is null-derived. Make sure the prefix is used in all cases.
I think it was done on purpose as (per [1]):
- "In addition, for null-derived capabilities (capabilities where the tag and upper attribute word are all zero), only the address is displayed (the Basic Format)."
[1] is indeed not very clear on the format in that case, but CheriBSD does print 0x for null-derived capabilities too [2] and so does Musl. It would be strange to print it only if the capability is not null-derived.
- Basic Format: ... "This should generally match the existing format used with integer pointers."
- "Linux kernel displays integer pointers as hexadecimal without a 0x prefix), and p should match a platform's existing format."
And I think the reason why "%#p" works is just a coincidence as '#' is being supported by relevant formatting function (number) which is used when printing default integer pointers. It is not by 'design' as such.
I don't have a strong opinion either for or against printing the prefix, but we should definitely be consistent (either always or never print it).
I think I would be more inclined to drop the prefix from the Simplified formatting - so no prefix in both cases: valid or null-derived. That is to be consistent with how Linux handles printing pointers. Though that's just my preference.
Having another look, it seems that skipping the prefix while displaying the address itself, might cause another confusion with how lower and upper bounds of a capability are presented (according to earlier mentioned spec those should include the '0x' prefix). Given that, it might indeed be better to display the prefix for all displayed capability formats and components.
Still, displaying capabilities through Basic Format (with "%p" specifier) should follow the format used on given platform when printing integer pointers - so on Linux this implies dropping the prefix. Null-derived capabilities should be displayed with Basic Format (despite the format specifier requested) - so without a prefix. In those cases capability metadata are being disregarded and only its address is being considered - being treated as integer pointer. So far this seems to be pretty consistent.
Now adding prefix while displaying the capability with Simplified Format might be perceived as lacking consistency wrt the prefix, but one could also argue that displaying the capability with this format considers all components of a capability and uses dedicated format specifier and as such may deviate from how displaying integer pointers (through "%p" specifier) is being handled on a given platform.
That said, if this stills present an issue, I have no objections to merging this change.
--- BR Beata
BR Beata
Kevin
[2] https://github.com/CTSRD-CHERI/cheribsd/blob/main/lib/libc/stdio/printfcommo...
[1] https://github.com/CTSRD-CHERI/cheri-c-programming/wiki/Displaying-Capabilit...
BR Beata
linux-morello mailing list -- linux-morello@op-lists.linaro.org To unsubscribe send an email to linux-morello-leave@op-lists.linaro.org
On 19/02/2024 20:14, Beata Michalska wrote:
On Thu, Feb 15, 2024 at 08:14:37AM +0100, Beata Michalska wrote:
On Fri, Feb 09, 2024 at 02:56:51PM +0000, Kevin Brodsky wrote:
On 08/02/2024 13:07, Beata Michalska wrote:
On Thu, Feb 08, 2024 at 09:29:26AM +0000, Kevin Brodsky wrote:
%#lp results in the address being prefixed with 0x, like %#p, but not if the capability is null-derived. Make sure the prefix is used in all cases.
I think it was done on purpose as (per [1]):
- "In addition, for null-derived capabilities (capabilities where the tag and upper attribute word are all zero), only the address is displayed (the Basic Format)."
[1] is indeed not very clear on the format in that case, but CheriBSD does print 0x for null-derived capabilities too [2] and so does Musl. It would be strange to print it only if the capability is not null-derived.
- Basic Format: ... "This should generally match the existing format used with integer pointers."
- "Linux kernel displays integer pointers as hexadecimal without a 0x prefix), and p should match a platform's existing format."
And I think the reason why "%#p" works is just a coincidence as '#' is being supported by relevant formatting function (number) which is used when printing default integer pointers. It is not by 'design' as such.
I don't have a strong opinion either for or against printing the prefix, but we should definitely be consistent (either always or never print it).
I think I would be more inclined to drop the prefix from the Simplified formatting - so no prefix in both cases: valid or null-derived. That is to be consistent with how Linux handles printing pointers. Though that's just my preference.
Having another look, it seems that skipping the prefix while displaying the address itself, might cause another confusion with how lower and upper bounds of a capability are presented (according to earlier mentioned spec those should include the '0x' prefix). Given that, it might indeed be better to display the prefix for all displayed capability formats and components.
Still, displaying capabilities through Basic Format (with "%p" specifier) should follow the format used on given platform when printing integer pointers
- so on Linux this implies dropping the prefix. Null-derived capabilities should
be displayed with Basic Format (despite the format specifier requested) - so without a prefix. In those cases capability metadata are being disregarded and only its address is being considered - being treated as integer pointer. So far this seems to be pretty consistent.
Now adding prefix while displaying the capability with Simplified Format might be perceived as lacking consistency wrt the prefix, but one could also argue that displaying the capability with this format considers all components of a capability and uses dedicated format specifier and as such may deviate from how displaying integer pointers (through "%p" specifier) is being handled on a given platform.
That said, if this stills present an issue, I have no objections to merging this change.
Agreed this is not clear cut. To me the main argument is that it is really strange for printk("%#p", ptr) to print 0x, while printk("%#lp", cap) doesn't if cap is null-derived (the format being otherwise the same, just the address). With that patch 0x is always printed when # is used. It may not have been the original intention, but from a user perspective consistency is best I think.
That patch is now in next, with a typo in the commit title fixed. I've also removed the Fixes: tag considering the discussion above - it's not really a bug, just a different interpretation of the expected format.
Kevin
linux-morello@op-lists.linaro.org
-
Beata Michalska -
Kevin Brodsky