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

[PATCH] 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.

To fix this bug, use a new variable 'iter' as the list iterator,
while use the origin variable 's' as a dedicated pointer to
point to the found element.

Cc: stable@vger.kernel.org
Fixes: ^1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Xiaomeng Tong <xiam0nd.tong@gmail.com>
---
 drivers/s390/char/tty3270.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/s390/char/tty3270.c b/drivers/s390/char/tty3270.c
index 5c83f71c1d0e..030e9a098d11 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,15 @@ 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 || s->len != flen) {
 		/* Reallocate string. */
 		n = tty3270_alloc_string(tp, flen);
 		list_add(&n->list, &s->list);
-- 
2.17.1

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

[Sven Schnelle]

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

> 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
                                                      bypass? ^^^^^

> the 'if (s->len != flen) {' in theory iif s->len's value is flen.
                                        ^^^ if?
>
> To fix this bug, use a new variable 'iter' as the list iterator,
> while use the origin variable 's' as a dedicated pointer to
using?  ^^^
        
> point to the found element.
>
> Cc: stable@vger.kernel.org
> Fixes: ^1da177e4c3f4 ("Linux-2.6.12-rc2")
> Signed-off-by: Xiaomeng Tong <xiam0nd.tong@gmail.com>
> ---
>  drivers/s390/char/tty3270.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/s390/char/tty3270.c b/drivers/s390/char/tty3270.c
> index 5c83f71c1d0e..030e9a098d11 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,15 @@ 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 || s->len != flen) {

This doesn't look right. You're checking for s == NULL here

>  		/* Reallocate string. */
>  		n = tty3270_alloc_string(tp, flen);
>  		list_add(&n->list, &s->list);

and if it is NULL, list_add() would be called here.

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

[Xiaomeng Tong]

On  Mon, 28 Mar 2022 08:01:02 +0200,  Sven Schnelle wrote:
> > diff --git a/drivers/s390/char/tty3270.c b/drivers/s390/char/tty3270.c
> > index 5c83f71c1d0e..030e9a098d11 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,15 @@ 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 || s->len != flen) {
> 
> This doesn't look right. You're checking for s == NULL here
> 
> >  		/* Reallocate string. */
> >  		n = tty3270_alloc_string(tp, flen);
> >  		list_add(&n->list, &s->list);
> 
> and if it is NULL, list_add() would be called here.

Yes, you are right, i have submitted PATCH v2 to fix it, thank you.

--
Xiaomeng Tong