[PATCH] powerpc/nvram: Fix Segmentation fault issue in nvram-size.

[Likhitha Korrapati]

nvram-size option results in segmentation fault when the user
specifies value larger than the default nvram size

Without the patch:
[root@xxx ~]# nvram --nvram-size 1048592
nvram: WARNING: expected 1048592 bytes, but only read 15360!
Segmentation fault (core dumped)

Segmentation fault is caused because the phead->length is becoming 0.
And because of this the p_start doesn't get updated which makes the
while loop run infinitely resulting in segmentation fault.
This patch adds a condition check for phead->length to avoid infinite
while loop.

With the patch:
[root@xxx src]# ./nvram --nvram-size 1048592
./nvram: WARNING: expected 1048592 bytes, but only read 15360!
[root@xxx src]# ./nvram --nvram-size 268435456
./nvram: WARNING: expected 268435456 bytes, but only read 15360!
[root@xxx src]#

Reported-by: Shirisha Ganta <shir...@linux.ibm.com>
Signed-off-by: Likhitha Korrapati <likh...@linux.ibm.com>
---
src/nvram.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/src/nvram.c b/src/nvram.c
index f051e9c..0d3d363 100644
--- a/src/nvram.c
+++ b/src/nvram.c
@@ -460,8 +460,12 @@ nvram_parse_partitions(struct nvram *nvram)
c_sum = checksum(phead);
if (c_sum != phead->checksum)
warn_msg("this partition checksum should be %02x!\n", c_sum);
- phead->length = be16toh(phead->length);
- p_start += phead->length * NVRAM_BLOCK_SIZE;
+ if (phead->length != 0) {
+ phead->length = be16toh(phead->length);
+ p_start += phead->length * NVRAM_BLOCK_SIZE;
+ }
+ else
+ break;
}

if (verbose)
--
2.39.3

[Shirisha ganta]

On Mon, 2023-09-11 at 05:23 -0500, Likhitha Korrapati wrote:
> nvram-size option results in segmentation fault when the user
> specifies value larger than the default nvram size
>
> Without the patch:
> [root@xxx ~]# nvram --nvram-size 1048592
> nvram: WARNING: expected 1048592 bytes, but only read 15360!
> Segmentation fault (core dumped)
>
> Segmentation fault is caused because the phead->length is becoming 0.
> And because of this the p_start doesn't get updated which makes the
> while loop run infinitely resulting in segmentation fault.
> This patch adds a condition check for phead->length to avoid infinite
> while loop.
>
> With the patch:
> [root@xxx src]# ./nvram --nvram-size 1048592
> ./nvram: WARNING: expected 1048592 bytes, but only read 15360!
> [root@xxx src]# ./nvram --nvram-size 268435456
> ./nvram: WARNING: expected 268435456 bytes, but only read 15360!
> [root@xxx src]#
>
> Reported-by: Shirisha Ganta <shir...@linux.ibm.com>
> Signed-off-by: Likhitha Korrapati <likh...@linux.ibm.com>

Thanks for the fix patch.
I have tested on Power10 machine, the segmentation fault is resolved
with the patch applied.

Output without patch
# nvram --nvram-size 268435456

nvram: WARNING: expected 268435456 bytes, but only read 15360!

Segmentation fault (core dumped)
#

Output with patch
# ./src/nvram --nvram-size 268435456
./src/nvram: WARNING: expected 268435456 bytes, but only read 15360!
#

Tested-by: Shirisha Ganta <shir...@linux.ibm.com>

[Tyrel Datwyler]

This else should be on the same line as the closing brace above. Also, best
practice is that if one conditional branch code block is braced all of the of
them should be.

ie. if ( ) {
...
} else {
...
}

I will fix these up on my end.

-Tyrel

[Tyrel Datwyler]

On 9/11/23 03:23, Likhitha Korrapati wrote:

> nvram-size option results in segmentation fault when the user
> specifies value larger than the default nvram size
>
> Without the patch:
> [root@xxx ~]# nvram --nvram-size 1048592
> nvram: WARNING: expected 1048592 bytes, but only read 15360!
> Segmentation fault (core dumped)
>
> Segmentation fault is caused because the phead->length is becoming 0.
> And because of this the p_start doesn't get updated which makes the
> while loop run infinitely resulting in segmentation fault.
> This patch adds a condition check for phead->length to avoid infinite
> while loop.
>
> With the patch:
> [root@xxx src]# ./nvram --nvram-size 1048592
> ./nvram: WARNING: expected 1048592 bytes, but only read 15360!
> [root@xxx src]# ./nvram --nvram-size 268435456
> ./nvram: WARNING: expected 268435456 bytes, but only read 15360!
> [root@xxx src]#
>
> Reported-by: Shirisha Ganta <shir...@linux.ibm.com>
> Signed-off-by: Likhitha Korrapati <likh...@linux.ibm.com>
> ---

Patch applied to powerpc-utils/next branch.

https://github.com/ibm-power-utilities/powerpc-utils/commit/a6d31caf4eaa453d3ec879f02163b3a515789b85

Thanks,
-Tyrel

[Tyrel Datwyler]

On 9/11/23 03:23, Likhitha Korrapati wrote:

> nvram-size option results in segmentation fault when the user
> specifies value larger than the default nvram size
>
> Without the patch:
> [root@xxx ~]# nvram --nvram-size 1048592
> nvram: WARNING: expected 1048592 bytes, but only read 15360!
> Segmentation fault (core dumped)
>
> Segmentation fault is caused because the phead->length is becoming 0.
> And because of this the p_start doesn't get updated which makes the
> while loop run infinitely resulting in segmentation fault.
> This patch adds a condition check for phead->length to avoid infinite
> while loop.
>
> With the patch:
> [root@xxx src]# ./nvram --nvram-size 1048592
> ./nvram: WARNING: expected 1048592 bytes, but only read 15360!
> [root@xxx src]# ./nvram --nvram-size 268435456
> ./nvram: WARNING: expected 268435456 bytes, but only read 15360!
> [root@xxx src]#

The following observation was made by Nathan Lynch:

The change in question here isn't really right. It avoids a crash, but
now results in a bogus entry at the end of the partition listing:

[root@ltc-zz14-lp1 ~]# ~test/powerpc-utils/src/nvram --partitions
# Sig Chk Len Name
0 50 ef 0024 of-config
1 70 99 009d common
2 a0 b5 0083 ibm,rtas-log
3 a0 4f 00fb lnx,oops-log
4 7f 9b 0181 wwwwwwwwwwww
[root@ltc-zz14-lp1 ~]# ~test/powerpc-utils/src/nvram --partitions --nvram-size
$((16 * 1024))
/home/test/powerpc-utils/src/nvram: WARNING: expected 16384 bytes, but only
read 15360!
# Sig Chk Len Name
0 50 ef 0024 of-config
1 70 99 009d common
2 a0 b5 0083 ibm,rtas-log
3 a0 4f 00fb lnx,oops-log
4 7f 9b 0181 wwwwwwwwwwww
5 00 00 0000 <<<<<<<<<<

As such I will likely revert this patch. The following issue has been raised to
evaluate how to move forward.

https://github.com/ibm-power-utilities/powerpc-utils/issues/96

-Tyrel

[Nathan Lynch]

I'd say let's not revert (it's better behavior than crashing). But let's
consider whether the --nvram-size option serves any real need.

[Tyrel Datwyler]

Yeah, I've been digging around and found this comment:

/* If we are using the DEFAULT_NVRAM_SZ value we to do a small bit of
* fixup here. All of the remaining code assumes that nbytes contains
* the actual size of nvram, not a guess-timated amount and bad things
* ensue if it is not correct.
*/

So, I think use of --nvram-size is intended as a last resort and unpredictable
results if you don't know what you are doing. Further, there is also the
following which probably accounts for the bogus entry you noted above:

if (remaining) {
warn_msg("expected %d bytes, but only read %d!\n",
nvram->nbytes, nvram->nbytes - remaining);
/* preserve the given nbytes, but zero the rest in case someone cares */
memset(p, 0, remaining);
}

-Tyrel