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

Jiri Slaby
2022-03-28T10:10:05Z

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