Hi Naveen, On 2026-09-01T10:23:25, Naveen Kumar Chaudhary <[email protected]> wrote: > bootstage: fix unchecked malloc and undersized buffer in bootstage_mark_code() > > bootstage_mark_code() allocated the label buffer without checking the > result and then dereferenced it, risking a NULL pointer crash on > allocation failure. The length calculation also failed to account for > the "," and ": " separators emitted by the snprintf() calls, so the > assembled string could be silently truncated. Additionally, when file > and func are NULL and linenum is -1, the buffer was passed on > uninitialized.
Please rewrite in present tense per U-Boot / Linux convention, e.g. 'allocates the label buffer without checking the result', 'fails to account for', 'is passed on uninitialised'. This patch aims to change the current code. Also worth noting that the only in-tree caller is the BOOTSTAGE_MARKER macro, which always passes __FILE__, __func__ and __LINE__, so the NULL/-1 case is theoretical hardening rather than an observed crash - the truncation and unchecked malloc() are the real fixes. > > Account for the separator bytes, bail out on allocation failure, and > ensure the buffer is always NUL-terminated. > > Signed-off-by: Naveen Kumar Chaudhary <[email protected]> > > common/bootstage.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > diff --git a/common/bootstage.c b/common/bootstage.c > @@ -175,12 +175,15 @@ ulong bootstage_mark_code(const char *file, const char > *func, int linenum) > if (linenum != -1) > len = 11; > if (func) > - len += strlen(func); > + len += strlen(func) + 2; /* ": " separator */ > if (file) > - len += strlen(file); > + len += strlen(file) + 1; /* "," separator */ BTW each snprintf() reserves a byte for its own terminating NUL within end - p, so the effective content budget is len - 1, not len. This only matters if %d expands to the full 11 chars (a negative int), which cannot happen for __LINE__, so it is not a real bug, just thought I'd mention it. > diff --git a/common/bootstage.c b/common/bootstage.c > @@ -175,12 +175,15 @@ ulong bootstage_mark_code(const char *file, const char > *func, int linenum) > str = malloc(len + 1); > + if (!str) > + return timer_get_boot_us(); Returning a valid-looking timestamp on allocation failure silently drops the record with no indication to the caller. Returning 0 (or at least a log_debug()) would be clearer - what do you think? Regards, Simon
