From: Josh Law <objecting@objecting.org>
The bounds check for brace_index happens after the array write.
While the current call pattern prevents an actual out-of-bounds
access (the previous call would have returned an error), the
write-before-check pattern is fragile and would become a real
out-of-bounds write if the error return were ever not propagated.
Move the bounds check before the array write so the function is
self-contained and safe regardless of caller behavior.
Signed-off-by: Josh Law <objecting@objecting.org>
---
lib/bootconfig.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lib/bootconfig.c b/lib/bootconfig.c
index a1e6a2e14b01..62b4ed7a0ba6 100644
--- a/lib/bootconfig.c
+++ b/lib/bootconfig.c
@@ -532,9 +532,9 @@ static char *skip_spaces_until_newline(char *p)
static int __init __xbc_open_brace(char *p)
{
/* Push the last key as open brace */
- open_brace[brace_index++] = xbc_node_index(last_parent);
if (brace_index >= XBC_DEPTH_MAX)
return xbc_parse_error("Exceed max depth of braces", p);
+ open_brace[brace_index++] = xbc_node_index(last_parent);
return 0;
}
--
2.34.1On Thu, 12 Mar 2026 19:11:42 +0000
Josh Law <hlcj1234567@gmail.com> wrote:
> From: Josh Law <objecting@objecting.org>
>
> The bounds check for brace_index happens after the array write.
> While the current call pattern prevents an actual out-of-bounds
> access (the previous call would have returned an error), the
> write-before-check pattern is fragile and would become a real
> out-of-bounds write if the error return were ever not propagated.
>
> Move the bounds check before the array write so the function is
> self-contained and safe regardless of caller behavior.
>
Hmm good catch. This is a kind of specification mistake.
If we pass below to the bootconfig tool, currently it fails.
----
key1 {
key2 {
key3 {
key4 {
key5 {
key6 {
key7 {
key8 {
key9 {
key10 {
key11 {
key12 {
key13 {
key14 {
key15 {
key16 {
}}}}}}}}}}}}}}}}
----
But since "open_brace[]" array has 16 entries, it should
accept the 16th brace.
Let me add a good and a bad example for this case too.
Thanks,
> Signed-off-by: Josh Law <objecting@objecting.org>
> ---
> lib/bootconfig.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/lib/bootconfig.c b/lib/bootconfig.c
> index a1e6a2e14b01..62b4ed7a0ba6 100644
> --- a/lib/bootconfig.c
> +++ b/lib/bootconfig.c
> @@ -532,9 +532,9 @@ static char *skip_spaces_until_newline(char *p)
> static int __init __xbc_open_brace(char *p)
> {
> /* Push the last key as open brace */
> - open_brace[brace_index++] = xbc_node_index(last_parent);
> if (brace_index >= XBC_DEPTH_MAX)
> return xbc_parse_error("Exceed max depth of braces", p);
> + open_brace[brace_index++] = xbc_node_index(last_parent);
>
> return 0;
> }
> --
> 2.34.1
>
--
Masami Hiramatsu (Google) <mhiramat@kernel.org>
On Thu, 12 Mar 2026 19:11:42 +0000
Josh Law <hlcj1234567@gmail.com> wrote:
> From: Josh Law <objecting@objecting.org>
>
> The bounds check for brace_index happens after the array write.
> While the current call pattern prevents an actual out-of-bounds
> access (the previous call would have returned an error), the
> write-before-check pattern is fragile and would become a real
> out-of-bounds write if the error return were ever not propagated.
>
> Move the bounds check before the array write so the function is
> self-contained and safe regardless of caller behavior.
This is the only place that increments the index, and the check is >=,
which means even if there was just one space left, it would fail.
As there's no other place that updates brace_index, I don't believe this
patch is needed. It could even replace the >= with ==.
-- Steve
>
> Signed-off-by: Josh Law <objecting@objecting.org>
> ---
> lib/bootconfig.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/lib/bootconfig.c b/lib/bootconfig.c
> index a1e6a2e14b01..62b4ed7a0ba6 100644
> --- a/lib/bootconfig.c
> +++ b/lib/bootconfig.c
> @@ -532,9 +532,9 @@ static char *skip_spaces_until_newline(char *p)
> static int __init __xbc_open_brace(char *p)
> {
> /* Push the last key as open brace */
> - open_brace[brace_index++] = xbc_node_index(last_parent);
> if (brace_index >= XBC_DEPTH_MAX)
> return xbc_parse_error("Exceed max depth of braces", p);
> + open_brace[brace_index++] = xbc_node_index(last_parent);
>
> return 0;
> }
12 Mar 2026 21:06:31 Steven Rostedt <rostedt@goodmis.org>:
> On Thu, 12 Mar 2026 19:11:42 +0000
> Josh Law <hlcj1234567@gmail.com> wrote:
>
>> From: Josh Law <objecting@objecting.org>
>>
>> The bounds check for brace_index happens after the array write.
>> While the current call pattern prevents an actual out-of-bounds
>> access (the previous call would have returned an error), the
>> write-before-check pattern is fragile and would become a real
>> out-of-bounds write if the error return were ever not propagated.
>>
>> Move the bounds check before the array write so the function is
>> self-contained and safe regardless of caller behavior.
>
> This is the only place that increments the index, and the check is >=,
> which means even if there was just one space left, it would fail.
>
> As there's no other place that updates brace_index, I don't believe this
> patch is needed. It could even replace the >= with ==.
>
> -- Steve
>
>
>>
>> Signed-off-by: Josh Law <objecting@objecting.org>
>> ---
>> lib/bootconfig.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/lib/bootconfig.c b/lib/bootconfig.c
>> index a1e6a2e14b01..62b4ed7a0ba6 100644
>> --- a/lib/bootconfig.c
>> +++ b/lib/bootconfig.c
>> @@ -532,9 +532,9 @@ static char *skip_spaces_until_newline(char *p)
>> static int __init __xbc_open_brace(char *p)
>> {
>> /* Push the last key as open brace */
>> - open_brace[brace_index++] = xbc_node_index(last_parent);
>> if (brace_index >= XBC_DEPTH_MAX)
>> return xbc_parse_error("Exceed max depth of braces", p);
>> + open_brace[brace_index++] = xbc_node_index(last_parent);
>>
>> return 0;
>> }
That's a fair point, Steve. Given that brace_index isn't touched elsewhere and the current check effectively prevents the overflow, I agree this isn't strictly necessary. I'll drop this patch and stick with the fix for the off-by-one reporting error instead. Thanks for the feedback!
12 Mar 2026 21:08:03 Josh Law <hlcj1234567@gmail.com>:
> 12 Mar 2026 21:06:31 Steven Rostedt <rostedt@goodmis.org>:
>
>> On Thu, 12 Mar 2026 19:11:42 +0000
>> Josh Law <hlcj1234567@gmail.com> wrote:
>>
>>> From: Josh Law <objecting@objecting.org>
>>>
>>> The bounds check for brace_index happens after the array write.
>>> While the current call pattern prevents an actual out-of-bounds
>>> access (the previous call would have returned an error), the
>>> write-before-check pattern is fragile and would become a real
>>> out-of-bounds write if the error return were ever not propagated.
>>>
>>> Move the bounds check before the array write so the function is
>>> self-contained and safe regardless of caller behavior.
>>
>> This is the only place that increments the index, and the check is >=,
>> which means even if there was just one space left, it would fail.
>>
>> As there's no other place that updates brace_index, I don't believe this
>> patch is needed. It could even replace the >= with ==.
>>
>> -- Steve
>>
>>
>>>
>>> Signed-off-by: Josh Law <objecting@objecting.org>
>>> ---
>>> lib/bootconfig.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/lib/bootconfig.c b/lib/bootconfig.c
>>> index a1e6a2e14b01..62b4ed7a0ba6 100644
>>> --- a/lib/bootconfig.c
>>> +++ b/lib/bootconfig.c
>>> @@ -532,9 +532,9 @@ static char *skip_spaces_until_newline(char *p)
>>> static int __init __xbc_open_brace(char *p)
>>> {
>>> /* Push the last key as open brace */
>>> - open_brace[brace_index++] = xbc_node_index(last_parent);
>>> if (brace_index >= XBC_DEPTH_MAX)
>>> return xbc_parse_error("Exceed max depth of braces", p);
>>> + open_brace[brace_index++] = xbc_node_index(last_parent);
>>>
>>> return 0;
>>> }
>
> That's a fair point, Steve. Given that brace_index isn't touched elsewhere and the current check effectively prevents the overflow, I agree this isn't strictly necessary. I'll drop this patch and stick with the fix for the off-by-one reporting error instead. Thanks for the feedback!
Wait Steve,
Thanks for the look. I see your point that it's currently redundant given the call patterns. It looks like Andrew has already merged this into the -mm tree, likely as a 'belt-and-suspenders' safety measure. I'll keep your feedback in mind for future cleanup, but I'm glad we got the other off-by-one fix in as well!
And in my opinion, merging it is a decent idea.
On Thu, 12 Mar 2026 21:09:52 +0000 Josh Law <hlcj1234567@gmail.com> wrote: > > That's a fair point, Steve. Given that brace_index isn't touched elsewhere and the current check effectively prevents the overflow, I agree this isn't strictly necessary. I'll drop this patch and stick with the fix for the off-by-one reporting error instead. Thanks for the feedback! > > Wait Steve, > Thanks for the look. I see your point that it's currently redundant given the call patterns. It looks like Andrew has already merged this into the -mm tree, likely as a 'belt-and-suspenders' safety measure. I'll keep your feedback in mind for future cleanup, but I'm glad we got the other off-by-one fix in as well! Please wordwrap the emails. > And in my opinion, merging it is a decent idea. You've changed your position without explaining why?
12 Mar 2026 21:28:11 Andrew Morton <akpm@linux-foundation.org>: > On Thu, 12 Mar 2026 21:09:52 +0000 Josh Law <hlcj1234567@gmail.com> wrote: > >>> That's a fair point, Steve. Given that brace_index isn't touched elsewhere and the current check effectively prevents the overflow, I agree this isn't strictly necessary. I'll drop this patch and stick with the fix for the off-by-one reporting error instead. Thanks for the feedback! >> >> Wait Steve, >> Thanks for the look. I see your point that it's currently redundant given the call patterns. It looks like Andrew has already merged this into the -mm tree, likely as a 'belt-and-suspenders' safety measure. I'll keep your feedback in mind for future cleanup, but I'm glad we got the other off-by-one fix in as well! > > Please wordwrap the emails. > >> And in my opinion, merging it is a decent idea. > > You've changed your position without explaining why? Sorry, I think it should be merged because it's better to be safe than sorry, I know there is different methods of implementation, but this one still works... I know it's churn (and I'm sorry)
On Thu, 12 Mar 2026 21:30:10 +0000 Josh Law <hlcj1234567@gmail.com> wrote: > 12 Mar 2026 21:28:11 Andrew Morton <akpm@linux-foundation.org>: > > > On Thu, 12 Mar 2026 21:09:52 +0000 Josh Law <hlcj1234567@gmail.com> wrote: > > > >>> That's a fair point, Steve. Given that brace_index isn't touched elsewhere and the current check effectively prevents the overflow, I agree this isn't strictly necessary. I'll drop this patch and stick with the fix for the off-by-one reporting error instead. Thanks for the feedback! > >> > >> Wait Steve, > >> Thanks for the look. I see your point that it's currently redundant given the call patterns. It looks like Andrew has already merged this into the -mm tree, likely as a 'belt-and-suspenders' safety measure. I'll keep your feedback in mind for future cleanup, but I'm glad we got the other off-by-one fix in as well! > > > > Please wordwrap the emails. > > > >> And in my opinion, merging it is a decent idea. > > > > You've changed your position without explaining why? > > Sorry, I think it should be merged because it's better to be safe than sorry, I know there is different methods of implementation, but this one still works... I know it's churn (and I'm sorry) I would like to keep this original >= because it is safer. Andrew, I will pick these patches with my test patch. Thank you, -- Masami Hiramatsu (Google) <mhiramat@kernel.org>
On Thu, 12 Mar 2026 21:30:10 +0000 Josh Law <hlcj1234567@gmail.com> wrote: > 12 Mar 2026 21:28:11 Andrew Morton <akpm@linux-foundation.org>: > > > On Thu, 12 Mar 2026 21:09:52 +0000 Josh Law <hlcj1234567@gmail.com> wrote: > > > >>> That's a fair point, Steve. Given that brace_index isn't touched > >>> elsewhere and the current check effectively prevents the overflow, I > >>> agree this isn't strictly necessary. I'll drop this patch and stick > >>> with the fix for the off-by-one reporting error instead. Thanks for > >>> the feedback! > >> > >> Wait Steve, > >> Thanks for the look. I see your point that it's currently redundant > >> given the call patterns. It looks like Andrew has already merged this > >> into the -mm tree, likely as a 'belt-and-suspenders' safety measure. > >> I'll keep your feedback in mind for future cleanup, but I'm glad we > >> got the other off-by-one fix in as well! > > > > Please wordwrap the emails. > > > >> And in my opinion, merging it is a decent idea. > > > > You've changed your position without explaining why? > /me wraps your email (claws-mail does that nicely ;-) > Sorry, I think it should be merged because it's better to be safe than > sorry, I know there is different methods of implementation, but this one > still works... I know it's churn (and I'm sorry) It's not the only place that does that. And since the value is safe as is, I rather not touch it. -- Steve
© 2016 - 2026 Red Hat, Inc.