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, WARN_ON and return.
Cc: stable@vger.kernel.org Fixes: ^1da177e4c3f4 ("Linux-2.6.12-rc2") 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. */
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,
Jiri Slaby jirislaby@kernel.org writes:
Cc: stable@vger.kernel.org Fixes: ^1da177e4c3f4 ("Linux-2.6.12-rc2")
That's barely the commit introducing the behavior.
Well, that code was introduced way before linux switch to git - not sure whether it makes sense to provide a Fixes: header in that case.
On Mon, 28 Mar 2022 12:09:59 +0200, Jiri Slaby wrote:
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?
When list_for_each_entry_* do not early exits (if the list is empty or no break/goto/return hit inside the loop), it will set pos ('s' here) with a bogus pointer that point to a invalid struct computed based on &HEAD using container_of.
#define list_for_each_entry(pos, head, member) \ for (pos = list_first_entry(head, typeof(*pos), member); \ !list_entry_is_head(pos, head, member); \ pos = list_next_entry(pos, member))
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.
Thank you, i will correct it.
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.
So just remove the Fixes tag? or something else? I find this commitID with git blame.
-- Xiaomeng Tong
On 28. 03. 22, 12:27, Xiaomeng Tong wrote:
On Mon, 28 Mar 2022 12:09:59 +0200, Jiri Slaby wrote:
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?
When list_for_each_entry_* do not early exits (if the list is empty or no break/goto/return hit inside the loop), it will set pos ('s' here) with a bogus pointer that point to a invalid struct computed based on &HEAD using container_of.
#define list_for_each_entry(pos, head, member) \ for (pos = list_first_entry(head, typeof(*pos), member); \ !list_entry_is_head(pos, head, member); \ pos = list_next_entry(pos, member))
No, I didn't mean what happens on that site on the code level. I think everyone understands that. Instead, I meant: what circumstances lead to this _situation_ in reality?
thanks,
linux-stable-mirror@lists.linaro.org
-
Jiri Slaby -
Sven Schnelle -
Xiaomeng Tong