Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Sven Schnelle <svens@linux.ibm.com>
To:     Xiaomeng Tong <xiam0nd.tong@gmail.com>
Cc:     hca@linux.ibm.com, gor@linux.ibm.com, agordeev@linux.ibm.com,
        borntraeger@linux.ibm.com, jirislaby@kernel.org,
        gregkh@linuxfoundation.org, jcmvbkbc@gmail.com, dsterba@suse.com,
        elder@linaro.org, linux-s390@vger.kernel.org,
        linux-kernel@vger.kernel.org, stable@vger.kernel.org
Subject: Re: [PATCH] char: tty3270: fix a missing check on list iterator
References: <20220327064931.7775-1-xiam0nd.tong@gmail.com>
Date:   Mon, 28 Mar 2022 08:01:02 +0200
In-Reply-To: <20220327064931.7775-1-xiam0nd.tong@gmail.com> (Xiaomeng Tong's
        message of "Sun, 27 Mar 2022 14:49:31 +0800")
Message-ID: <yt9dmthad3a9.fsf@linux.ibm.com>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/28.0.50 (gnu/linux)
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

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.