Re: [PATCH] printk: add cpu id information to printk() output
On Fri 2023-09-22 15:34:44, Enlin Mu wrote: > John Ogness 于2023年9月15日周五 16:34写道： > > > > On 2023-09-15, Enlin Mu wrote: > > > Sometimes we want to print cpu id of printk() messages to consoles > > > > > > diff --git a/include/linux/threads.h b/include/linux/threads.h > > > index c34173e6c5f1..6700bd9a174f 100644 > > > --- a/include/linux/threads.h > > > +++ b/include/linux/threads.h > > > @@ -34,6 +34,9 @@ > > > #define PID_MAX_LIMIT (CONFIG_BASE_SMALL ? PAGE_SIZE * 8 : \ > > > (sizeof(long) > 4 ? 4 * 1024 * 1024 : PID_MAX_DEFAULT)) > > > > > > +#define CPU_ID_SHIFT 23 > > > +#define CPU_ID_MASK 0xff80 > > > > This only supports 256 CPUs. I think it doesn't make sense to try to > > squish CPU and Task IDs into 32 bits. > > > > What about introducing a caller_id option to always only print the CPU > > ID? Or do you really need Task _and_ CPU? > > Yes, I need it. > For SOC manufacturer, sometimes cpu is not stable, we need some debug > tools for this exceptions. > When an exception occurs, we may not be able to detect it in a timely > manner, but through Task _and_ CPU, we can roughly locate the CPU at > the time of the exception. Would frace be enough in this case? It has already been suggested earlier. The problem is that adding CPU into would require changes in the metadata stored in the ring buffer. And it would require updating userspace tools which read the log from a crash dump. It means that such a feature would need a lot of effort. And I would prefer to avoid it when there is another solution. If you debug the problem by adding extra printk messages you might also print the current CPU in the debug messages. Best Regards, Petr
Re: [PATCH] printk: add cpu id information to printk() output
On Fri 2023-09-22 15:20:37, Enlin Mu wrote:
> Petr Mladek 于2023年9月16日周六 00:34写道：
> >
> > On Fri 2023-09-15 11:53:13, Greg KH wrote:
> > > On Fri, Sep 15, 2023 at 04:46:02PM +0800, Enlin Mu wrote:
> > > > John Ogness 于2023年9月15日周五 16:34写道：
> > > > >
> > > > > On 2023-09-15, Enlin Mu wrote:
> > > > > > Sometimes we want to print cpu id of printk() messages to consoles
> > > > > >
> > > > > > diff --git a/include/linux/threads.h b/include/linux/threads.h
> > > > > > index c34173e6c5f1..6700bd9a174f 100644
> > > > > > --- a/include/linux/threads.h
> > > > > > +++ b/include/linux/threads.h
> > > > > > @@ -34,6 +34,9 @@
> > > > > > #define PID_MAX_LIMIT (CONFIG_BASE_SMALL ? PAGE_SIZE * 8 : \
> > > > > > (sizeof(long) > 4 ? 4 * 1024 * 1024 : PID_MAX_DEFAULT))
> > > > > >
> > > > > > +#define CPU_ID_SHIFT 23
> > > > > > +#define CPU_ID_MASK 0xff80
> > > > >
> > > > > This only supports 256 CPUs. I think it doesn't make sense to try to
> > > > > squish CPU and Task IDs into 32 bits.
> > > > Yes, it is not good way,
> > > > >
> > > > > What about introducing a caller_id option to always only print the CPU
> > > > > ID? Or do you really need Task _and_ CPU?
> > > >Yes, I need it.Because I need to know which CPU is printing the
> > > > log, so that I can identify the current system operation, such as load
> > > > situation and CPU busy/idle status
> > >
> > > The cpu that is printing the log isn't the one that added the log
> > > message, so I think you will have incorrect data here, right?
> >
> > We already store some metadata about the caller:
> >
> > * All fields are set by the printk code except for @seq, which is
> > * set by the ringbuffer code.
> > */
> > struct printk_info {
> > u64 seq;/* sequence number */
> > u64 ts_nsec;/* timestamp in nanoseconds */
> > u16 text_len; /* length of text message */
> > u8 facility; /* syslog facility */
> > u8 flags:5;/* internal record flags */
> > u8 level:3;/* syslog level */
> > u32 caller_id; /* thread id or processor id */
> >
> > struct dev_printk_info dev_info;
> > };
> >
> > The 32-bit caller ID is generated using:
> >
> > static inline u32 printk_caller_id(void)
> > {
> > return in_task() ? task_pid_nr(current) :
> > 0x8000 + smp_processor_id();
> > }
> >
> > We could add more metadata and always store the CPU ID and something
> > like:
> >
> >[CTXT][ Tpid][ Ccpu]
> >
> > for example
> >
> >[TASK][ T234][C4]
> >[ IRQ][ T4567][ C17]
> >[SIRQ][T5][C0]
> >[ NMI][ T356][ C128]
> >
> Greate!
> Do you have a plan to push it to linus?
No. It was just a POC. It would require much more effort to make
it ready for upstream, see below.
> > The biggest problem is that it would change the format of the
> > ringbuffer so that it would require updating external tools,
> > working with crashdump, especially crash but there are also
> > alternative python extensions for gdb.
It would require patches for the crash tool,
./scripts/gdb/linux/dmesg.py,
Documentation/admin-guide/kdump/gdbmacros.txt
> > See below POC of the kernel part. It is not even compile tested. The size
> > of the buffers is updated by a guess. Comments are not updated, ...
And of course, make the POC working, update comments, ...
I am sorry but I do not have enough time and motivation to do so.
But I could answer questions, review the patches, ... when any
interested person start working on it.
Best Regards,
Petr
Re: Suggestion for Capability Check Refinement in check_syslog_permissions()
On Wed 2024-01-03 07:59:18, Greg KH wrote:
> On Wed, Jan 03, 2024 at 01:00:58PM +0800, 孟敬姿 wrote:
> > Hi, we suggest revisiting the capability checks in
> > check_syslog_permissions(). Currently CAP_SYSLOG is checked first, and
> > if it’s not there but there is a CAP_SYS_ADMIN, it can also pass the
> > check. We recommend refining this check to exclusively use CAP_SYSLOG.
> > Here's our reasoning for this suggestion:
>
> Again, have you tested this?
I guess that Meng is right. The current code looks like:
static int check_syslog_permissions(int type, int source)
{
/*
* If this is from /proc/kmsg and we've already opened it, then we've
* already done the capabilities checks at open time.
*/
if (source == SYSLOG_FROM_PROC && type != SYSLOG_ACTION_OPEN)
goto ok;
if (syslog_action_restricted(type)) {
if (capable(CAP_SYSLOG))
goto ok;
/*
* For historical reasons, accept CAP_SYS_ADMIN too, with
* a warning.
*/
if (capable(CAP_SYS_ADMIN)) {
pr_warn_once("%s (%d): Attempt to access syslog with "
"CAP_SYS_ADMIN but no CAP_SYSLOG "
"(deprecated).\n",
current->comm, task_pid_nr(current));
goto ok;
}
return -EPERM;
}
ok:
return security_syslog(type);
}
And CAP_SYS_ADMIN has really been deprecated last 13 years, see the
commit ee24aebffb75a7f940cf ("cap_syslog: accept CAP_SYS_ADMIN for now").
Maybe, it is really time to remove it.
Best Regards,
Petr
Re: [PATCH] cap_syslog: remove CAP_SYS_ADMIN when dmesg_restrict
On Fri 2024-01-05 09:49:44, Theodore Ts'o wrote:
> On Fri, Jan 05, 2024 at 02:20:07PM +0800, Jingzi Meng wrote:
> > CAP_SYSLOG was separated from CAP_SYS_ADMIN and introduced in Linux
> > 2.6.37 (2010-11). For a long time, certain syslog actions required
> > CAP_SYS_ADMIN or CAP_SYSLOG. Maybe it’s time to officially remove
> > CAP_SYS_ADMIN for more fine-grained control.
> >
> > CAP_SYS_ADMIN was once removed but added back for backwards
> > compatibility reasons. In commit 38ef4c2e437d ("syslog: check cap_syslog
> > when dmesg_restrict") (2010-12), CAP_SYS_ADMIN was no longer needed. And
> > in commit ee24aebffb75 ("cap_syslog: accept CAP_SYS_ADMIN for now")
> > (2011-02), it was accepted again. Since then, CAP_SYS_ADMIN has been
> > preserved.
> >
> > Now that almost 13 years have passed, the legacy application may have
> > had enough time to be updated.
>
> What testing have you done to make sure that this is OK? "May have
> had enough time"? That's not very reassuring?
>
> Also, note that we can't actually reuse the bit position of
> CAP_SYS_ADMIN since it's likely that there may be pre-existing
> capability masks that are still using that position. So there isn't
> all that much upside in trying to retire CAP_SYS_ADMIN --- if you as a
> system administrator think it's not too course, then just don't use
> it.
>
> It's unclear to me what goal you have in trying to mess with the
> capability definitions? Perhaps it might be useful if you were to
> explicitly state your goals in these proposals?
My understanding is that this patch is about reducing overlap of
capabilities.
Allowing the same thing with more capabilities seems to go against
the idea of separate capabilities.
Kernel has printed the warning for 13 years. It is a long
time to fix configuration for newly installed systems. And I doubt
that anyone is installing a new kernel on 13 year's old system.
IMHO, this fits into the category that it should be OK until
anyone complains. But I might miss something.
Best Regards,
Petr
Re: [PATCH 1/1] lib/vsprintf: Implement ssprintf() to catch truncated strings
On Tue 2024-01-30 15:53:36, Lee Jones wrote: > On Tue, 30 Jan 2024, Rasmus Villemoes wrote: > > On 30/01/2024 16.07, Lee Jones wrote: > > > On Mon, 29 Jan 2024, Lee Jones wrote: > > >> On Mon, 29 Jan 2024, David Laight wrote: > > snprintf() does this and has been proven to cause buffer-overflows. > > There have been multiple articles authored describing why using > > snprintf() is not generally a good idea for the masses including the 2 > > linked in the commit message: > > >>> > > >>> snprintf() returns the number of bytes that would have been output [1]. > > > > Okay, I've written a bunch of simple test cases and results are > > > positive. It seems to achieve my aim whilst minimising any potential > > > pitfalls. > > > > > > - Success returns Bytes actually written - no functional change > > > - Overflow returns the size of the buffer - which makes the result > > >a) testable for overflow > > >b) non-catastrophic if accidentally used to manipulate later sizes > > > > You are describing scnprintf(), which almost does exactly that. The last > > thing we need is another interface with almost identical semantics. > > It does, which is why when I first centred my efforts on this task the > plan was to simply switch to it. However, as I described in the commit > message: > > "Whist executing the task, it quickly became apparent that the initial > thought of simply s/snprintf/scnprintf/ wasn't going to be adequate > for a number of cases. Specifically ones where the caller needs to > know whether the given string ends up being truncated." > > A great deal of callers want to know if the string they attempted to > form was successful. A malformed string would lead to oddities in the > best cases and various device/probing/matching failures in the worst. > > > > int size = 10; > > > char buf[size]; > > > char *b = buf; > > > > > > ret = spprintf(b, size, "1234"); > > > size -= ret; > > > b += ret; > > > // ret = 4 size = 6 buf = "1234\0" > > > > > > ret = spprintf(b, size, "5678"); > > > size -= ret; > > > b += ret; > > > // ret = 4 size = 2 buf = "12345678\0" > > > > > > ret = spprintf(b, size, "9***"); > > > size -= ret; > > > b += ret; > > > // ret = 2 size = 0 buf = "123456789\0" > > > > So here scnprint() would have returned 1, leaving size at 1. scnprintf() > > has the invariant that, for non-zero size, the return value is strictly > > less than that size, so when passed a size of 1, all subsequent calls > > return 0 (corresponding to the fact that all it could do was to write > > the '\0' terminator). > > > > This pattern already exists, and is really the reason scnprint exists. > > Yes, scnprintf() cannot distinguish overflow from > > it-just-exactly-fitted. Maybe it would have been better to make it work > > like this, but I don't think there's a real use > > There are real use-cases. They are what brought me here. > > > and we do have > > seq_buf() if one really wants an interface that can build a string > > piece-meal while keeping track of whether it ever caused overflow. > > seq_buf_*() looks okay, but it's petty heavy requiring what looks like > the buffers to be initialised with an API call before use. We're > looking for something more light weight. > > scnprint() had clear safety centric improvements over snprintf() and > spprintf() adds an additional layer of return value checking on top of > that. > > I'm not sure I understand the resistance to something which is needed > and has clear benefits over what presently exists just for the sake of a > few lines of code. I'd be on board if it were change for the sake of > change, but the added flexibility and ease of use is evident. My view is the following: First, I agree that snprintf() is dangerous and should be replaced. I think that the resistance is because: + ssprintf() has its danger as well. You wrote [1] that "Under-running the buffer is no worse over-running". But it is no better either. + More APIs might create more confusion and increase the risk of a misuse. + spprintf() does not allow to detect truncated string. It means that it does not provide any advantage over the existing scnprintf(). So, if we could solve it another way then it might be better. That said, people tend to look how an API is used instead of RTFM. They copy good or bad patterns. There is even a term for this but I can't remember it new. So, if we introduce a new API and provide some good examples then there is a good chance that people will continue using it correctly. And it even might be checked more easily. There is a similarity with alloc() APIs. Good (most) programmers know that they need to check kalloc() return value. They might also learn that they need to check also return value from ssnprintf() of how we call it. Especially, when we provide good examples from the very beginning. Also it might be checke
Re: [PATCH] printk: cleanup deprecated uses of strncpy/strcpy
On Mon 2024-04-29 23:06:54, Justin Stitt wrote:
> Cleanup some deprecated uses of strncpy() and strcpy() [1].
>
> There doesn't seem to be any bugs with the current code but the
> readability of this code could benefit from a quick makeover while
> removing some deprecated stuff as a benefit.
>
> The most interesting replacement made in this patch involves
> concatenating "ttyS" with a digit-led user-supplied string. Instead of
> doing two distinct string copies with carefully managed offsets and
> lengths, let's use the more robust and self-explanatory scnprintf().
> scnprintf will 1) respect the bounds of @buf, 2) null-terminate @buf, 3)
> do the concatenation. This allows us to drop the manual NUL-byte assignment.
>
> Also, since isdigit() is used about a dozen lines after the open-coded
> version we'll replace it for uniformity's sake.
>
> All the strcpy() --> strscpy() replacements are trivial as the source
> strings are literals and much smaller than the destination size. No
> behavioral change here.
>
> Use the new 2-argument version of strscpy() introduced in Commit
> e6584c3964f2f ("string: Allow 2-argument strscpy()"). However, to make
> this work fully (since the size must be known at compile time), also
> update the extern-qualified declaration to have the proper size
> information.
>
> Link:
> https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings
> [1]
> Link: https://github.com/KSPP/linux/issues/90 [2]
> Link: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html
> [3]
> Cc: linux-hardening@vger.kernel.org
> Signed-off-by: Justin Stitt
Nice improvements. Looks fine.
Reviewed-by: Petr Mladek
Best Regards,
Petr
Re: [PATCH] printk: cleanup deprecated uses of strncpy/strcpy
On Thu 2024-05-02 07:06:21, Christophe JAILLET wrote:
> Le 02/05/2024 à 01:18, Justin Stitt a écrit :
> > On Wed, May 1, 2024 at 2:39 PM Christophe JAILLET
> > wrote:
> > > Hi,
> > >
> > > Nit: The { } around each branch can now also be removed.
> >
> > There was one line before and there's one line now.
>
> In the block after the "else", yes, but now the block after the "if" is only
> 1 line. (it was 2 before).
>
> So, {} should now be omitted on both branches.
>
> -if (str[0] >= '0' && str[0] <= '9') {
> -strcpy(buf, "ttyS");
> -strncpy(buf + 4, str, sizeof(buf) - 5);
> +if (isdigit(str[0])) {
> +scnprintf(buf, sizeof(buf), "ttyS%s", str);
> } else {
> -strncpy(buf, str, sizeof(buf) - 1);
> +strscpy(buf, str);
> }
>
> This is a really minor nitpick. Not sure you need to repost if there is no
> other comment.
I could remove the brackets when pushing the patch. But feel free to
send v2.
I am going to push it the following week if nobody complains.
Best Regards,
Petr
Re: [PATCH] printk: cleanup deprecated uses of strncpy/strcpy
On Mon 2024-04-29 23:06:54, Justin Stitt wrote:
> Cleanup some deprecated uses of strncpy() and strcpy() [1].
>
> There doesn't seem to be any bugs with the current code but the
> readability of this code could benefit from a quick makeover while
> removing some deprecated stuff as a benefit.
>
> The most interesting replacement made in this patch involves
> concatenating "ttyS" with a digit-led user-supplied string. Instead of
> doing two distinct string copies with carefully managed offsets and
> lengths, let's use the more robust and self-explanatory scnprintf().
> scnprintf will 1) respect the bounds of @buf, 2) null-terminate @buf, 3)
> do the concatenation. This allows us to drop the manual NUL-byte assignment.
>
> Also, since isdigit() is used about a dozen lines after the open-coded
> version we'll replace it for uniformity's sake.
>
> All the strcpy() --> strscpy() replacements are trivial as the source
> strings are literals and much smaller than the destination size. No
> behavioral change here.
>
> Use the new 2-argument version of strscpy() introduced in Commit
> e6584c3964f2f ("string: Allow 2-argument strscpy()"). However, to make
> this work fully (since the size must be known at compile time), also
> update the extern-qualified declaration to have the proper size
> information.
>
> Link:
> https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings
> [1]
> Link: https://github.com/KSPP/linux/issues/90 [2]
> Link: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html
> [3]
> Cc: linux-hardening@vger.kernel.org
> Signed-off-by: Justin Stitt
JFYI, the patch has been comitted into printk/linux.git, branch for-6.10.
I have removed the obsoleted brackets and added some empty lines
to break the blob of code a bit, see
https://git.kernel.org/pub/scm/linux/kernel/git/printk/linux.git/commit/?h=for-6.10&id=e0550222e03bae3fd629641e246ef7f47803d795
Best Regards,
Petr
Re: [PATCH v2] printf: Remove unused 'bprintf'
On Wed 2024-10-02 18:31:47, li...@treblig.org wrote:
> From: "Dr. David Alan Gilbert"
>
> bprintf() is unused. Remove it. It was added in the commit 4370aa4aa753
> ("vsprintf: add binary printf") but as far as I can see was never used,
> unlike the other two functions in that patch.
>
> Signed-off-by: Dr. David Alan Gilbert
> Reviewed-by: Andy Shevchenko
Looks good to me:
Acked-by: Petr Mladek
I assume that Sven is going to take it via the ftrace tree as he
suggested at
https://lore.kernel.org/r/20241002104807.42b4b...@gandalf.local.home
Best Regards,
Petr
Re: [DISCUSSION] vsprintf: the current state of restricted pointers (%pK)
On Tue 2025-01-14 16:35:57, Andy Shevchenko wrote:
> On Mon, Jan 13, 2025 at 05:46:44PM +0100, Thomas Weißschuh wrote:
> > Hi everybody,
> >
> > as you know, leaking raw kernel pointers to the user is problematic as
> > they can be used to break KASLR.
> > Therefore back in 2011 the %pK format specifier was added [0], printing
> > certain pointers zeroed out or raw depending on the usage context.
> > Then in 2017 even the default %p format was changed to hash the pointers
> > [1].
> >
> > Both mechanisms are similar in their intention but have different,
> > cross-interacting effects and configuration knobs.
> > The end result is not always obvious. For example:
> > * "no_hash_pointers" does not work for %pK if kernel.kptr_restrict>=1
> > * If kernel.kptr_restrict=1, "restricted" pointers are effectively
> > less restricted than "normal" pointers.
> > * For other values of kernel.kptr_restrict %p and %pK have the same
> > security properties, but still different string representations.
> >
> > Additionally the current usage of %pK is incorrect in many cases.
> > As %pK relies on the current task context for its permission check, it
> > was only ever meant to be used from procfs/sysfs/debugfs handlers [2].
> > In reality many callers use it through printk(), leaking addresses
> > into dmesg. While restricted_pointer() tries to detect some of such
> > situations at runtime, this check is not and can not be always complete.
> >
> > File handlers which could use %pK correctly today, often use
> > kallsyms_show_value() instead. This is similar, but checks explicitly
> > against the credentials from an opened file instead of the implicit task
> > credentials. This behavior was the goal for %pK all along [3].
>
> > Is it time to inspect the users of %pK and migrate them to either
> > %p/%px, kallsyms_show_value() or some similar new API?
> > Then alias %pK to %p, maybe removing it at some point.
>
> To me this paragraph sounds like a good plan, which I agree on!
+1
> > A different, but slightly related issue occurs with PREEMPT_RT.
> > Calling printk("%pK") while holding a raw spinlock will trigger an
> > invalid wait context and latency spikes if an LSM using sleeping
> > spinlocks is enabled.
> > As printk() should be callable from any context this is an issue.
> > Removing the implicit group check would also avoid this.
Good to know.
Best Regards,
Petr
