linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] s390/qeth: fix a missing-check bug
@ 2018-10-06 16:08 Wenwen Wang
  2018-10-08  5:01 ` Heiko Carstens
  0 siblings, 1 reply; 2+ messages in thread
From: Wenwen Wang @ 2018-10-06 16:08 UTC (permalink / raw)
  To: Wenwen Wang
  Cc: Kangjie Lu, Julian Wiedmann, Ursula Braun, Martin Schwidefsky,
	Heiko Carstens, open list:S390 NETWORK DRIVERS, open list

In qeth_snmp_command(), the length of the user request is firstly copied
from the user-space buffer 'udata' to the kernel variable 'req_len' and
checked to see whether it is too large. If the check fails, an error code
EINVAL is returned. Otherwise, the execution continues and the whole buffer
is copied again from 'udata' and saved to the kernel buffer 'ureq'.
However, after the second copy, no re-check is enforced on the newly-copied
request length. Given that the buffer 'udata' is in the user space, a
malicious user can race to change the request length between the two
copies. In this way, the attacker can supply malicious data to the kernel
and cause undefined behavior.

This patch adds a re-check on the request length after the second copy from
the buffer 'udata'. If the newly-copied value is different from the value
obtained in the first copy, i.e., 'req_len', an error code EINVAL will be
returned after the buffer 'ureq' is freed.

Signed-off-by: Wenwen Wang <wang6495@umn.edu>
---
 drivers/s390/net/qeth_core_main.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/s390/net/qeth_core_main.c b/drivers/s390/net/qeth_core_main.c
index de82824..6199743 100644
--- a/drivers/s390/net/qeth_core_main.c
+++ b/drivers/s390/net/qeth_core_main.c
@@ -4613,6 +4613,10 @@ static int qeth_snmp_command(struct qeth_card *card, char __user *udata)
 		QETH_CARD_TEXT(card, 2, "snmpnome");
 		return PTR_ERR(ureq);
 	}
+	if (ureq->hdr.req_len != req_len) {
+		kfree(ureq);
+		return -EINVAL;
+	}
 	qinfo.udata_len = ureq->hdr.data_len;
 	qinfo.udata = kzalloc(qinfo.udata_len, GFP_KERNEL);
 	if (!qinfo.udata) {
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 2+ messages in thread

* Re: [PATCH] s390/qeth: fix a missing-check bug
  2018-10-06 16:08 [PATCH] s390/qeth: fix a missing-check bug Wenwen Wang
@ 2018-10-08  5:01 ` Heiko Carstens
  0 siblings, 0 replies; 2+ messages in thread
From: Heiko Carstens @ 2018-10-08  5:01 UTC (permalink / raw)
  To: Wenwen Wang
  Cc: Kangjie Lu, Julian Wiedmann, Ursula Braun, Martin Schwidefsky,
	open list:S390 NETWORK DRIVERS, open list

On Sat, Oct 06, 2018 at 11:08:23AM -0500, Wenwen Wang wrote:
> In qeth_snmp_command(), the length of the user request is firstly copied
> from the user-space buffer 'udata' to the kernel variable 'req_len' and
> checked to see whether it is too large. If the check fails, an error code
> EINVAL is returned. Otherwise, the execution continues and the whole buffer
> is copied again from 'udata' and saved to the kernel buffer 'ureq'.
> However, after the second copy, no re-check is enforced on the newly-copied
> request length. Given that the buffer 'udata' is in the user space, a
> malicious user can race to change the request length between the two
> copies. In this way, the attacker can supply malicious data to the kernel
> and cause undefined behavior.
> 
> This patch adds a re-check on the request length after the second copy from
> the buffer 'udata'. If the newly-copied value is different from the value
> obtained in the first copy, i.e., 'req_len', an error code EINVAL will be
> returned after the buffer 'ureq' is freed.
> 
> Signed-off-by: Wenwen Wang <wang6495@umn.edu>
> ---
>  drivers/s390/net/qeth_core_main.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/s390/net/qeth_core_main.c b/drivers/s390/net/qeth_core_main.c
> index de82824..6199743 100644
> --- a/drivers/s390/net/qeth_core_main.c
> +++ b/drivers/s390/net/qeth_core_main.c
> @@ -4613,6 +4613,10 @@ static int qeth_snmp_command(struct qeth_card *card, char __user *udata)
>  		QETH_CARD_TEXT(card, 2, "snmpnome");
>  		return PTR_ERR(ureq);
>  	}
> +	if (ureq->hdr.req_len != req_len) {
> +		kfree(ureq);
> +		return -EINVAL;
> +	}

ureq->hdr.req_len is not used anywhere in the code, so could you please
explain what the undefined behavior is?

You could argue that adding a second sanity check may help to avoid
potential future bugs, but currently the code looks sane to me.


^ permalink raw reply	[flat|nested] 2+ messages in thread

end of thread, other threads:[~2018-10-08  5:01 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-10-06 16:08 [PATCH] s390/qeth: fix a missing-check bug Wenwen Wang
2018-10-08  5:01 ` Heiko Carstens

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).