Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
Message-ID: <47a6e396-3d51-79f5-a544-8942470fa2fd@kernel.org>
Date:   Mon, 28 Mar 2022 12:09:59 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.6.0
Subject: Re: [PATCH v3] char: tty3270: fix a missing check on list iterator
Content-Language: en-US
To:     Xiaomeng Tong <xiam0nd.tong@gmail.com>, hca@linux.ibm.com,
        gor@linux.ibm.com, agordeev@linux.ibm.com
Cc:     borntraeger@linux.ibm.com, svens@linux.ibm.com,
        gregkh@linuxfoundation.org, jcmvbkbc@gmail.com, elder@linaro.org,
        dsterba@suse.com, linux-s390@vger.kernel.org,
        linux-kernel@vger.kernel.org, stable@vger.kernel.org
References: <20220328093505.27902-1-xiam0nd.tong@gmail.com>
From:   Jiri Slaby <jirislaby@kernel.org>
In-Reply-To: <20220328093505.27902-1-xiam0nd.tong@gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 28. 03. 22, 11:35, Xiaomeng Tong wrote:
> 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.

Could you also explain how that can happen?

> 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,

bpass + iif -- others already commented on that and you ignored them.

> 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, WARN_ON and return.
> 
> Cc: stable@vger.kernel.org
> Fixes: ^1da177e4c3f4 ("Linux-2.6.12-rc2")

That's barely the commit introducing the behavior.

> Signed-off-by: Xiaomeng Tong <xiam0nd.tong@gmail.com>
> ---
> changes since v2:
>   - WARN_ON and return (Sven Schnelle)
> 
> changes since v1:
>   - reallocate s when s == NULL (Sven Schnelle)
> 
> v1:https://lore.kernel.org/lkml/20220327064931.7775-1-xiam0nd.tong@gmail.com/
> v2:https://lore.kernel.org/lkml/20220328070543.24671-1-xiam0nd.tong@gmail.com/
> 
> ---
>   drivers/s390/char/tty3270.c | 11 ++++++++---
>   1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/s390/char/tty3270.c b/drivers/s390/char/tty3270.c
> index 5c83f71c1d0e..9d0952178322 100644
> --- a/drivers/s390/char/tty3270.c
> +++ b/drivers/s390/char/tty3270.c
> @@ -1109,9 +1109,9 @@ static void tty3270_put_character(struct tty3270 *tp, char ch)
>   static void
>   tty3270_convert_line(struct tty3270 *tp, int line_nr)
>   {
> +	struct string *s = NULL, *n, *iter;
>   	struct tty3270_line *line;
>   	struct tty3270_cell *cell;
> -	struct string *s, *n;
>   	unsigned char highlight;
>   	unsigned char f_color;
>   	char *cp;
> @@ -1142,9 +1142,14 @@ 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;
> +		 }
> +
> +	if(WARN_ON(!s))
> +		return;
>   	/*
>   	 * Check if the line needs to get reallocated.
>   	 */

thanks,
-- 
js
suse labs