[PATCH v2] char: tty3270: fix a missing check on list iterator

[PATCH v2] char: tty3270: fix a missing check on list iterator

[Xiaomeng Tong]

The bug is here:
	if (s->len != flen) {

The list iterator 's' will point to a bogus position containing
HEAD if the list is empty or no element is found. This case must
be checked before any use of the iterator, otherwise it may bpass
the 'if (s->len != flen) {' in theory iif s->len's value is flen,
or/and lead to an invalid memory access.

To fix this bug, use a new variable 'iter' as the list iterator,
while using the origin variable 's' as a dedicated pointer to
point to the found element. And if the list is empty or no element
is found, reallocate s.

Cc: stable@vger.kernel.org
Fixes: ^1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Xiaomeng Tong <xiam0nd.tong@gmail.com>
---

changes since v1:
 - reallocate s when s == NULL (Sven Schnelle)

v1:https://lore.kernel.org/lkml/20220327064931.7775-1-xiam0nd.tong@gmail.com/

---
 drivers/s390/char/tty3270.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/drivers/s390/char/tty3270.c b/drivers/s390/char/tty3270.c
index 5c83f71c1d0e..719e04dff63e 100644
--- a/drivers/s390/char/tty3270.c
+++ b/drivers/s390/char/tty3270.c
@@ -1111,7 +1111,7 @@ tty3270_convert_line(struct tty3270 *tp, int line_nr)
 {
 	struct tty3270_line *line;
 	struct tty3270_cell *cell;
-	struct string *s, *n;
+	struct string *s = NULL, *n, *iter;
 	unsigned char highlight;
 	unsigned char f_color;
 	char *cp;
@@ -1142,13 +1142,20 @@ tty3270_convert_line(struct tty3270 *tp, int line_nr)
 
 	/* Find the line in the list. */
 	i = tp->view.rows - 2 - line_nr;
-	list_for_each_entry_reverse(s, &tp->lines, list)
-		if (--i <= 0)
+	list_for_each_entry_reverse(iter, &tp->lines, list)
+		if (--i <= 0) {
+			s = iter;
 			break;
+		}
 	/*
 	 * Check if the line needs to get reallocated.
 	 */
-	if (s->len != flen) {
+	if (!s) {
+		/* Reallocate string. */
+		n = tty3270_alloc_string(tp, flen);
+		list_add(&n->list, &tp->lines);
+		s = n;
+	} else if (s->len != flen) {
 		/* Reallocate string. */
 		n = tty3270_alloc_string(tp, flen);
 		list_add(&n->list, &s->list);
-- 
2.17.1

Re: [PATCH v2] char: tty3270: fix a missing check on list iterator

[Sven Schnelle]

Xiaomeng Tong <xiam0nd.tong@gmail.com> writes:

> --- a/drivers/s390/char/tty3270.c
> +++ b/drivers/s390/char/tty3270.c
> @@ -1111,7 +1111,7 @@ tty3270_convert_line(struct tty3270 *tp, int line_nr)
>  {
>  	struct tty3270_line *line;
>  	struct tty3270_cell *cell;
> -	struct string *s, *n;
> +	struct string *s = NULL, *n, *iter;

Please keep reverse XMAS-tree layout.

>  	unsigned char highlight;
>  	unsigned char f_color;
>  	char *cp;
> @@ -1142,13 +1142,20 @@ tty3270_convert_line(struct tty3270 *tp, int line_nr)
>  
>  	/* Find the line in the list. */
>  	i = tp->view.rows - 2 - line_nr;
> -	list_for_each_entry_reverse(s, &tp->lines, list)
> -		if (--i <= 0)
> +	list_for_each_entry_reverse(iter, &tp->lines, list)
> +		if (--i <= 0) {
> +			s = iter;
>  			break;
> +		}
>  	/*
>  	 * Check if the line needs to get reallocated.
>  	 */
> -	if (s->len != flen) {
> +	if (!s) {
> +		/* Reallocate string. */
> +		n = tty3270_alloc_string(tp, flen);
> +		list_add(&n->list, &tp->lines);
> +		s = n;
> +	} else if (s->len != flen) {
>  		/* Reallocate string. */
>  		n = tty3270_alloc_string(tp, flen);
>  		list_add(&n->list, &s->list);

I should have written that in my first reply, but s == NULL means
the given line number couldn't be found in the list of lines. This is
a serious error and should be warned about. So maybe something like:

if (WARN_ON(!s))
	return;

But allocating a new empty line in that case is certainly wrong.

Thanks
Sven

Re: [PATCH v2] char: tty3270: fix a missing check on list iterator

[Xiaomeng Tong]

> I should have written that in my first reply, but s == NULL means
> the given line number couldn't be found in the list of lines. This is
> a serious error and should be warned about. So maybe something like:
> 
> if (WARN_ON(!s))
> 	return;
> 
> But allocating a new empty line in that case is certainly wrong.

Thank you, i have resend a v3 patch as you suggested.

--
Xiaomeng Tong