[PATCH v2] cmdline: prevent out-of-bounds read in completion buffer

[PATCH v2] cmdline: prevent out-of-bounds read in completion buffer

[Daniil Iskhakov]

tmp_buf is populated by the completion callback and is not guaranteed
to be NUL-terminated.

The code already accounts for this when computing tmp_size with
strnlen(tmp_buf, sizeof(tmp_buf)). However, another loop in the same
path still walks tmp_buf until a NUL byte is found, without checking
the buffer limit.

If the callback writes a full-sized non-NUL-terminated string, the loop
may read past the end of tmp_buf.

Fix this by bounding the iteration with sizeof(tmp_buf).

Found by Linux Verification Center (linuxtesting.org) with SVACE.

Fixes: af75078fece3 ("first public release")
Cc: stable@dpdk.org

Signed-off-by: Daniil Iskhakov <dish@amicon.ru>
---
v2:
- Resent to dev@dpdk.org because v1 was accidentally sent only to
maintainers.

Cc: sdl.dpdk@linuxtesting.org
Cc: rrv@amicon.ru
---
 lib/cmdline/cmdline_rdline.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/cmdline/cmdline_rdline.c b/lib/cmdline/cmdline_rdline.c
index ee070f0af3..bc91dc6002 100644
--- a/lib/cmdline/cmdline_rdline.c
+++ b/lib/cmdline/cmdline_rdline.c
@@ -445,7 +445,7 @@ rdline_char_in(struct rdline *rdl, char c)
 				rdline_puts(rdl, "\r\n");
 				while (ret) {
 					rdl->write_char(rdl, ' ');
-					for (i=0 ; i < sizeof(tmp_buf) && tmp_buf[i]; i++)
+					for (i = 0 ; i < tmp_buf[i]; i++)
 						rdl->write_char(rdl, tmp_buf[i]);
 					rdline_puts(rdl, "\r\n");
 					ret = rdl->complete(rdl, rdl->left_buf,
-- 
2.43.0

RE: [PATCH v2] cmdline: prevent out-of-bounds read in completion buffer

[Konstantin Ananyev]

> tmp_buf is populated by the completion callback and is not guaranteed
> to be NUL-terminated.
> 
> The code already accounts for this when computing tmp_size with
> strnlen(tmp_buf, sizeof(tmp_buf)). However, another loop in the same
> path still walks tmp_buf until a NUL byte is found, without checking
> the buffer limit.
> 
> If the callback writes a full-sized non-NUL-terminated string, the loop
> may read past the end of tmp_buf.
> 
> Fix this by bounding the iteration with sizeof(tmp_buf).
> 
> Found by Linux Verification Center (linuxtesting.org) with SVACE.
> 
> Fixes: af75078fece3 ("first public release")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Daniil Iskhakov <dish@amicon.ru>
> ---
> v2:
> - Resent to dev@dpdk.org because v1 was accidentally sent only to
> maintainers.
> 
> Cc: sdl.dpdk@linuxtesting.org
> Cc: rrv@amicon.ru
> ---
>  lib/cmdline/cmdline_rdline.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/cmdline/cmdline_rdline.c b/lib/cmdline/cmdline_rdline.c
> index ee070f0af3..bc91dc6002 100644
> --- a/lib/cmdline/cmdline_rdline.c
> +++ b/lib/cmdline/cmdline_rdline.c
> @@ -445,7 +445,7 @@ rdline_char_in(struct rdline *rdl, char c)
>  				rdline_puts(rdl, "\r\n");
>  				while (ret) {
>  					rdl->write_char(rdl, ' ');
> -					for (i=0 ; i < sizeof(tmp_buf) &&
> tmp_buf[i]; i++)
> +					for (i = 0 ; i < tmp_buf[i]; i++)

> Fix this by bounding the iteration with sizeof(tmp_buf).
The change doesn't much description, if fact it looks contrary.
Probably patch get screwed somehow?

>  						rdl->write_char(rdl, tmp_buf[i]);
>  					rdline_puts(rdl, "\r\n");
>  					ret = rdl->complete(rdl, rdl->left_buf,
> --
> 2.43.0

[PATCH v3] cmdline: prevent out-of-bounds read in completion buffer

[Daniil Iskhakov]

tmp_buf is populated by the completion callback and is not guaranteed
to be NUL-terminated.

The code already accounts for this when computing tmp_size with
strnlen(tmp_buf, sizeof(tmp_buf)). However, another loop in the same
path still walks tmp_buf until a NUL byte is found, without checking
the buffer limit.

If the callback writes a full-sized non-NUL-terminated string, the loop
may read past the end of tmp_buf.

Fix this by computing a bounded length for each completion choice before
printing it.

Found by Linux Verification Center (linuxtesting.org) with SVACE.

Fixes: af75078fece3 ("first public release")
Cc: stable@dpdk.org

Signed-off-by: Daniil Iskhakov <dish@amicon.ru>
---
v3:
- Reworked the choice-printing loop to use a bounded length.
- Fixed coding style issues in the diff.
- Rebased and regenerated the patch; v2 did not apply cleanly.

v2:
- Resent to dev@dpdk.org because v1 was accidentally sent only to
  maintainers.

Cc: sdl.dpdk@linuxtesting.org
Cc: rrv@amicon.ru
---
 lib/cmdline/cmdline_rdline.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lib/cmdline/cmdline_rdline.c b/lib/cmdline/cmdline_rdline.c
index 0a5a399b32..e1770f0a1d 100644
--- a/lib/cmdline/cmdline_rdline.c
+++ b/lib/cmdline/cmdline_rdline.c
@@ -444,8 +444,9 @@ rdline_char_in(struct rdline *rdl, char c)
 				/* choice */
 				rdline_puts(rdl, "\r\n");
 				while (ret) {
+					tmp_size = strnlen(tmp_buf, sizeof(tmp_buf));
 					rdl->write_char(rdl, ' ');
-					for (i=0 ; tmp_buf[i] ; i++)
+					for (i = 0; i < tmp_size; i++)
 						rdl->write_char(rdl, tmp_buf[i]);
 					rdline_puts(rdl, "\r\n");
 					ret = rdl->complete(rdl, rdl->left_buf,
-- 
2.43.0