[PATCH 1/1] libavb: fix avb_replace() OOM handling

[PATCH 1/1] libavb: fix avb_replace() OOM handling

[Josh Law]

avb_replace() promises NULL on OOM. Once it had built the first
replacement, a later allocation failure returned that partial buffer.
Callers treat any result as success, so AVB could keep booting with
truncated bootargs.

Free the partial result and return NULL. The existing callers can then
take their OOM path.

Signed-off-by: Josh Law <josh2@disroot.org>
---
 lib/libavb/avb_util.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/lib/libavb/avb_util.c b/lib/libavb/avb_util.c
index 8719ede15a7..9e2e6ea3495 100644
--- a/lib/libavb/avb_util.c
+++ b/lib/libavb/avb_util.c
@@ -272,7 +272,7 @@ char* avb_replace(const char* str, const char* search, const char* replace) {
       num_new = num_before + replace_len + 1;
       ret = avb_malloc(num_new);
       if (ret == NULL) {
-        goto out;
+        goto fail;
       }
       avb_memcpy(ret, str, num_before);
       avb_memcpy(ret + num_before, replace, replace_len);
@@ -283,7 +283,7 @@ char* avb_replace(const char* str, const char* search, const char* replace) {
       num_new = ret_len + num_before + replace_len + 1;
       new_str = avb_malloc(num_new);
       if (new_str == NULL) {
-        goto out;
+        goto fail;
       }
       avb_memcpy(new_str, ret, ret_len);
       avb_memcpy(new_str + ret_len, str, num_before);
@@ -308,7 +308,7 @@ char* avb_replace(const char* str, const char* search, const char* replace) {
     size_t num_new = ret_len + num_remaining + 1;
     char* new_str = avb_malloc(num_new);
     if (new_str == NULL) {
-      goto out;
+      goto fail;
     }
     avb_memcpy(new_str, ret, ret_len);
     avb_memcpy(new_str + ret_len, str_after_last_replace, num_remaining);
@@ -320,6 +320,10 @@ char* avb_replace(const char* str, const char* search, const char* replace) {
 
 out:
   return ret;
+
+fail:
+  avb_free(ret);
+  return NULL;
 }
 
 /* We only support a limited amount of strings in avb_strdupv(). */
-- 
2.47.3

Re: [PATCH 1/1] libavb: fix avb_replace() OOM handling

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 2280 bytes --]

On Thu, May 21, 2026 at 04:32:48PM +0000, Josh Law wrote:
> avb_replace() promises NULL on OOM. Once it had built the first
> replacement, a later allocation failure returned that partial buffer.
> Callers treat any result as success, so AVB could keep booting with
> truncated bootargs.
> 
> Free the partial result and return NULL. The existing callers can then
> take their OOM path.
> 
> Signed-off-by: Josh Law <josh2@disroot.org>
> ---
>  lib/libavb/avb_util.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/libavb/avb_util.c b/lib/libavb/avb_util.c
> index 8719ede15a7..9e2e6ea3495 100644
> --- a/lib/libavb/avb_util.c
> +++ b/lib/libavb/avb_util.c
> @@ -272,7 +272,7 @@ char* avb_replace(const char* str, const char* search, const char* replace) {
>        num_new = num_before + replace_len + 1;
>        ret = avb_malloc(num_new);
>        if (ret == NULL) {
> -        goto out;
> +        goto fail;
>        }
>        avb_memcpy(ret, str, num_before);
>        avb_memcpy(ret + num_before, replace, replace_len);
> @@ -283,7 +283,7 @@ char* avb_replace(const char* str, const char* search, const char* replace) {
>        num_new = ret_len + num_before + replace_len + 1;
>        new_str = avb_malloc(num_new);
>        if (new_str == NULL) {
> -        goto out;
> +        goto fail;
>        }
>        avb_memcpy(new_str, ret, ret_len);
>        avb_memcpy(new_str + ret_len, str, num_before);
> @@ -308,7 +308,7 @@ char* avb_replace(const char* str, const char* search, const char* replace) {
>      size_t num_new = ret_len + num_remaining + 1;
>      char* new_str = avb_malloc(num_new);
>      if (new_str == NULL) {
> -      goto out;
> +      goto fail;
>      }
>      avb_memcpy(new_str, ret, ret_len);
>      avb_memcpy(new_str + ret_len, str_after_last_replace, num_remaining);
> @@ -320,6 +320,10 @@ char* avb_replace(const char* str, const char* search, const char* replace) {
>  
>  out:
>    return ret;
> +
> +fail:
> +  avb_free(ret);
> +  return NULL;
>  }
>  
>  /* We only support a limited amount of strings in avb_strdupv(). */

Thanks for the explanation and patch. This seems fine but I'll defer to
Mattijs as it's his area.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 1/1] libavb: fix avb_replace() OOM handling

[Simon Glass]

On 2026-05-21T16:51:21, Josh Law <josh2@disroot.org> wrote:
> libavb: fix avb_replace() OOM handling
>
> avb_replace() promises NULL on OOM. Once it had built the first
> replacement, a later allocation failure returned that partial buffer.
> Callers treat any result as success, so AVB could keep booting with
> truncated bootargs.
>
> Free the partial result and return NULL. The existing callers can then
> take their OOM path.
>
> Signed-off-by: Josh Law <josh2@disroot.org>
>
> lib/libavb/avb_util.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

Re: [PATCH 1/1] libavb: fix avb_replace() OOM handling

[Mattijs Korpershoek]

Hi Josh,

On Fri, May 22, 2026 at 12:35, Tom Rini <trini@konsulko.com> wrote:

> On Thu, May 21, 2026 at 04:32:48PM +0000, Josh Law wrote:
>> avb_replace() promises NULL on OOM. Once it had built the first
>> replacement, a later allocation failure returned that partial buffer.
>> Callers treat any result as success, so AVB could keep booting with
>> truncated bootargs.
>> 
>> Free the partial result and return NULL. The existing callers can then
>> take their OOM path.
>> 
>> Signed-off-by: Josh Law <josh2@disroot.org>
>> ---
>>  lib/libavb/avb_util.c | 10 +++++++---
>>  1 file changed, 7 insertions(+), 3 deletions(-)
>> 
>> diff --git a/lib/libavb/avb_util.c b/lib/libavb/avb_util.c
>> index 8719ede15a7..9e2e6ea3495 100644
>> --- a/lib/libavb/avb_util.c
>> +++ b/lib/libavb/avb_util.c
>> @@ -272,7 +272,7 @@ char* avb_replace(const char* str, const char* search, const char* replace) {
>>        num_new = num_before + replace_len + 1;
>>        ret = avb_malloc(num_new);
>>        if (ret == NULL) {
>> -        goto out;
>> +        goto fail;
>>        }
>>        avb_memcpy(ret, str, num_before);
>>        avb_memcpy(ret + num_before, replace, replace_len);
>> @@ -283,7 +283,7 @@ char* avb_replace(const char* str, const char* search, const char* replace) {
>>        num_new = ret_len + num_before + replace_len + 1;
>>        new_str = avb_malloc(num_new);
>>        if (new_str == NULL) {
>> -        goto out;
>> +        goto fail;
>>        }
>>        avb_memcpy(new_str, ret, ret_len);
>>        avb_memcpy(new_str + ret_len, str, num_before);
>> @@ -308,7 +308,7 @@ char* avb_replace(const char* str, const char* search, const char* replace) {
>>      size_t num_new = ret_len + num_remaining + 1;
>>      char* new_str = avb_malloc(num_new);
>>      if (new_str == NULL) {
>> -      goto out;
>> +      goto fail;
>>      }
>>      avb_memcpy(new_str, ret, ret_len);
>>      avb_memcpy(new_str + ret_len, str_after_last_replace, num_remaining);
>> @@ -320,6 +320,10 @@ char* avb_replace(const char* str, const char* search, const char* replace) {
>>  
>>  out:
>>    return ret;
>> +
>> +fail:
>> +  avb_free(ret);
>> +  return NULL;
>>  }
>>  
>>  /* We only support a limited amount of strings in avb_strdupv(). */
>
> Thanks for the explanation and patch. This seems fine but I'll defer to
> Mattijs as it's his area.

This patch seems to posted a second time here:
https://lore.kernel.org/all/20260521165122.17475-1-josh2@disroot.org/

Can you explain why it has been send twice, please?

>
> -- 
> Tom

Re: [PATCH 1/1] libavb: fix avb_replace() OOM handling

[Josh Law]

On May 26, 2026 2:17:08 PM GMT+01:00, Mattijs Korpershoek
<mkorpershoek@kernel.org> wrote:
>Hi Josh,
>
>On Fri, May 22, 2026 at 12:35, Tom Rini <trini@konsulko.com> wrote:
>
>> On Thu, May 21, 2026 at 04:32:48PM +0000, Josh Law wrote:
>>> avb_replace() promises NULL on OOM. Once it had built the first
>>> replacement, a later allocation failure returned that partial buffer.
>>> Callers treat any result as success, so AVB could keep booting with
>>> truncated bootargs.
>>> 
>>> Free the partial result and return NULL. The existing callers can then
>>> take their OOM path.
>>> 
>>> Signed-off-by: Josh Law <josh2@disroot.org>
>>> ---
>>>  lib/libavb/avb_util.c | 10 +++++++---
>>>  1 file changed, 7 insertions(+), 3 deletions(-)
>>> 
>>> diff --git a/lib/libavb/avb_util.c b/lib/libavb/avb_util.c
>>> index 8719ede15a7..9e2e6ea3495 100644
>>> --- a/lib/libavb/avb_util.c
>>> +++ b/lib/libavb/avb_util.c
>>> @@ -272,7 +272,7 @@ char* avb_replace(const char* str, const char*
>search, const char* replace) {
>>>        num_new = num_before + replace_len + 1;
>>>        ret = avb_malloc(num_new);
>>>        if (ret == NULL) {
>>> -        goto out;
>>> +        goto fail;
>>>        }
>>>        avb_memcpy(ret, str, num_before);
>>>        avb_memcpy(ret + num_before, replace, replace_len);
>>> @@ -283,7 +283,7 @@ char* avb_replace(const char* str, const char*
>search, const char* replace) {
>>>        num_new = ret_len + num_before + replace_len + 1;
>>>        new_str = avb_malloc(num_new);
>>>        if (new_str == NULL) {
>>> -        goto out;
>>> +        goto fail;
>>>        }
>>>        avb_memcpy(new_str, ret, ret_len);
>>>        avb_memcpy(new_str + ret_len, str, num_before);
>>> @@ -308,7 +308,7 @@ char* avb_replace(const char* str, const char*
>search, const char* replace) {
>>>      size_t num_new = ret_len + num_remaining + 1;
>>>      char* new_str = avb_malloc(num_new);
>>>      if (new_str == NULL) {
>>> -      goto out;
>>> +      goto fail;
>>>      }
>>>      avb_memcpy(new_str, ret, ret_len);
>>>      avb_memcpy(new_str + ret_len, str_after_last_replace,
>num_remaining);
>>> @@ -320,6 +320,10 @@ char* avb_replace(const char* str, const char*
>search, const char* replace) {
>>>  
>>>  out:
>>>    return ret;
>>> +
>>> +fail:
>>> +  avb_free(ret);
>>> +  return NULL;
>>>  }
>>>  
>>>  /* We only support a limited amount of strings in avb_strdupv(). */
>>
>> Thanks for the explanation and patch. This seems fine but I'll defer to
>> Mattijs as it's his area.
>
>This patch seems to posted a second time here:
>https://lore.kernel.org/all/20260521165122.17475-1-josh2@disroot.org/
>
>Can you explain why it has been send twice, please?

Heh. The like guardian of the "first post" lists, took a while, so I
subscribed to the list, thought that would get it approved on-list

It happens. Honest mistake :)


>>
>> -- 
>> Tom
>

Thanks!