[PATCH net v2] net/x25: prevent a couple of overflows

kernel-janitors.vger.kernel.org archive mirror
 help / color / mirror / Atom feed

* [PATCH net v2] net/x25: prevent a couple of overflows
@ 2020-12-01 15:15 ` Dan Carpenter
  2020-12-02  7:43   ` [PATCH net v2] net/x25: prevent a couple of overflows(Internet mail) kiyin(尹亮)
  2020-12-02  9:27   ` [PATCH net v2] net/x25: prevent a couple of overflows Martin Schiller
  0 siblings, 2 replies; 4+ messages in thread
From: Dan Carpenter @ 2020-12-01 15:15 UTC (permalink / raw)
  To: kernel-janitors

The .x25_addr[] address comes from the user and is not necessarily
NUL terminated.  This leads to a couple problems.  The first problem is
that the strlen() in x25_bind() can read beyond the end of the buffer.

The second problem is more subtle and could result in memory corruption.
The call tree is:
  x25_connect()
  --> x25_write_internal()
      --> x25_addr_aton()

The .x25_addr[] buffers are copied to the "addresses" buffer from
x25_write_internal() so it will lead to stack corruption.

Verify that the strings are NUL terminated and return -EINVAL if they
are not.

Reported-by: "kiyin(尹亮)" <kiyin@tencent.com>
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
The first patch put a NUL terminator on the end of the string and this
patch returns an error instead.  I don't have a strong preference, which
patch to go with.

 net/x25/af_x25.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/net/x25/af_x25.c b/net/x25/af_x25.c
index 9232cdb42ad9..d41fffb2507b 100644
--- a/net/x25/af_x25.c
+++ b/net/x25/af_x25.c
@@ -675,7 +675,8 @@ static int x25_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len)
 	int len, i, rc = 0;
 
 	if (addr_len != sizeof(struct sockaddr_x25) ||
-	    addr->sx25_family != AF_X25) {
+	    addr->sx25_family != AF_X25 ||
+	    strnlen(addr->sx25_addr.x25_addr, X25_ADDR_LEN) = X25_ADDR_LEN) {
 		rc = -EINVAL;
 		goto out;
 	}
@@ -769,7 +770,8 @@ static int x25_connect(struct socket *sock, struct sockaddr *uaddr,
 
 	rc = -EINVAL;
 	if (addr_len != sizeof(struct sockaddr_x25) ||
-	    addr->sx25_family != AF_X25)
+	    addr->sx25_family != AF_X25 ||
+	    strnlen(addr->sx25_addr.x25_addr, X25_ADDR_LEN) = X25_ADDR_LEN)
 		goto out;
 
 	rc = -ENETUNREACH;
-- 
2.29.2

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

* RE: [PATCH net v2] net/x25: prevent a couple of overflows(Internet mail)
  2020-12-01 15:15 ` [PATCH net v2] net/x25: prevent a couple of overflows Dan Carpenter
@ 2020-12-02  7:43   ` kiyin(尹亮)
  2020-12-02  9:27   ` [PATCH net v2] net/x25: prevent a couple of overflows Martin Schiller
  1 sibling, 0 replies; 4+ messages in thread
From: kiyin(尹亮) @ 2020-12-02  7:43 UTC (permalink / raw)
  To: Dan Carpenter, Martin Schiller
  Cc: David S. Miller, Jakub Kicinski, linux-x25, netdev,
	Andrew Hendry, security, linux-distros,
	huntchen(陈阳), dannywang(王宇),
	kernel-janitors

SGkgRGFuLA0KICAgIEkgdGhpbmsgdGhlIHN0cm5sZW4gaXMgYmV0dGVyLiB0aGUga2VybmVsIGRv
ZXNuJ3QgbmVlZCB0byBhZGp1c3QgdXNlciBsYW5kIG1pc3Rha2UgYnkgcHV0dGluZyBhIE5VTEwg
dGVybWluYXRvci4ganVzdCByZXR1cm4gYW4gZXJyb3IgdG8gbGV0IHRoZSB1c2VyIGxhbmQgcHJv
Z3JhbSBmaXggdGhlIHdyb25nIGFkZHJlc3MuDQoNClJlZ2FyZHMsDQpraXlpbg0KDQo+IC0tLS0t
T3JpZ2luYWwgTWVzc2FnZS0tLS0tDQo+IEZyb206IERhbiBDYXJwZW50ZXIgW21haWx0bzpkYW4u
Y2FycGVudGVyQG9yYWNsZS5jb21dDQo+IFNlbnQ6IFR1ZXNkYXksIERlY2VtYmVyIDEsIDIwMjAg
MTE6MTUgUE0NCj4gVG86IE1hcnRpbiBTY2hpbGxlciA8bXNAZGV2LnRkdC5kZT4NCj4gQ2M6IERh
dmlkIFMuIE1pbGxlciA8ZGF2ZW1AZGF2ZW1sb2Z0Lm5ldD47IEpha3ViIEtpY2luc2tpIDxrdWJh
QGtlcm5lbC5vcmc+Ow0KPiBsaW51eC14MjVAdmdlci5rZXJuZWwub3JnOyBuZXRkZXZAdmdlci5r
ZXJuZWwub3JnOyBBbmRyZXcgSGVuZHJ5DQo+IDxhbmRyZXcuaGVuZHJ5QGdtYWlsLmNvbT47IGtp
eWluKOWwueS6rikgPGtpeWluQHRlbmNlbnQuY29tPjsNCj4gc2VjdXJpdHlAa2VybmVsLm9yZzsg
bGludXgtZGlzdHJvc0B2cy5vcGVud2FsbC5vcmc7IGh1bnRjaGVuKOmZiOmYsykNCj4gPGh1bnRj
aGVuQHRlbmNlbnQuY29tPjsgZGFubnl3YW5nKOeOi+WuhykgPGRhbm55d2FuZ0B0ZW5jZW50LmNv
bT47DQo+IGtlcm5lbC1qYW5pdG9yc0B2Z2VyLmtlcm5lbC5vcmcNCj4gU3ViamVjdDogW1BBVENI
IG5ldCB2Ml0gbmV0L3gyNTogcHJldmVudCBhIGNvdXBsZSBvZiBvdmVyZmxvd3MoSW50ZXJuZXQg
bWFpbCkNCj4gDQo+IFRoZSAueDI1X2FkZHJbXSBhZGRyZXNzIGNvbWVzIGZyb20gdGhlIHVzZXIg
YW5kIGlzIG5vdCBuZWNlc3NhcmlseSBOVUwNCj4gdGVybWluYXRlZC4gIFRoaXMgbGVhZHMgdG8g
YSBjb3VwbGUgcHJvYmxlbXMuICBUaGUgZmlyc3QgcHJvYmxlbSBpcyB0aGF0IHRoZQ0KPiBzdHJs
ZW4oKSBpbiB4MjVfYmluZCgpIGNhbiByZWFkIGJleW9uZCB0aGUgZW5kIG9mIHRoZSBidWZmZXIu
DQo+IA0KPiBUaGUgc2Vjb25kIHByb2JsZW0gaXMgbW9yZSBzdWJ0bGUgYW5kIGNvdWxkIHJlc3Vs
dCBpbiBtZW1vcnkgY29ycnVwdGlvbi4NCj4gVGhlIGNhbGwgdHJlZSBpczoNCj4gICB4MjVfY29u
bmVjdCgpDQo+ICAgLS0+IHgyNV93cml0ZV9pbnRlcm5hbCgpDQo+ICAgICAgIC0tPiB4MjVfYWRk
cl9hdG9uKCkNCj4gDQo+IFRoZSAueDI1X2FkZHJbXSBidWZmZXJzIGFyZSBjb3BpZWQgdG8gdGhl
ICJhZGRyZXNzZXMiIGJ1ZmZlciBmcm9tDQo+IHgyNV93cml0ZV9pbnRlcm5hbCgpIHNvIGl0IHdp
bGwgbGVhZCB0byBzdGFjayBjb3JydXB0aW9uLg0KPiANCj4gVmVyaWZ5IHRoYXQgdGhlIHN0cmlu
Z3MgYXJlIE5VTCB0ZXJtaW5hdGVkIGFuZCByZXR1cm4gLUVJTlZBTCBpZiB0aGV5IGFyZSBub3Qu
DQo+IA0KPiBSZXBvcnRlZC1ieTogImtpeWluKOWwueS6rikiIDxraXlpbkB0ZW5jZW50LmNvbT4N
Cj4gU2lnbmVkLW9mZi1ieTogRGFuIENhcnBlbnRlciA8ZGFuLmNhcnBlbnRlckBvcmFjbGUuY29t
Pg0KPiAtLS0NCj4gVGhlIGZpcnN0IHBhdGNoIHB1dCBhIE5VTCB0ZXJtaW5hdG9yIG9uIHRoZSBl
bmQgb2YgdGhlIHN0cmluZyBhbmQgdGhpcyBwYXRjaA0KPiByZXR1cm5zIGFuIGVycm9yIGluc3Rl
YWQuICBJIGRvbid0IGhhdmUgYSBzdHJvbmcgcHJlZmVyZW5jZSwgd2hpY2ggcGF0Y2ggdG8gZ28N
Cj4gd2l0aC4NCj4gDQo+ICBuZXQveDI1L2FmX3gyNS5jIHwgNiArKysrLS0NCj4gIDEgZmlsZSBj
aGFuZ2VkLCA0IGluc2VydGlvbnMoKyksIDIgZGVsZXRpb25zKC0pDQo+IA0KPiBkaWZmIC0tZ2l0
IGEvbmV0L3gyNS9hZl94MjUuYyBiL25ldC94MjUvYWZfeDI1LmMgaW5kZXgNCj4gOTIzMmNkYjQy
YWQ5Li5kNDFmZmZiMjUwN2IgMTAwNjQ0DQo+IC0tLSBhL25ldC94MjUvYWZfeDI1LmMNCj4gKysr
IGIvbmV0L3gyNS9hZl94MjUuYw0KPiBAQCAtNjc1LDcgKzY3NSw4IEBAIHN0YXRpYyBpbnQgeDI1
X2JpbmQoc3RydWN0IHNvY2tldCAqc29jaywgc3RydWN0DQo+IHNvY2thZGRyICp1YWRkciwgaW50
IGFkZHJfbGVuKQ0KPiAgCWludCBsZW4sIGksIHJjID0gMDsNCj4gDQo+ICAJaWYgKGFkZHJfbGVu
ICE9IHNpemVvZihzdHJ1Y3Qgc29ja2FkZHJfeDI1KSB8fA0KPiAtCSAgICBhZGRyLT5zeDI1X2Zh
bWlseSAhPSBBRl9YMjUpIHsNCj4gKwkgICAgYWRkci0+c3gyNV9mYW1pbHkgIT0gQUZfWDI1IHx8
DQo+ICsJICAgIHN0cm5sZW4oYWRkci0+c3gyNV9hZGRyLngyNV9hZGRyLCBYMjVfQUREUl9MRU4p
ID09DQo+IFgyNV9BRERSX0xFTikgew0KPiAgCQlyYyA9IC1FSU5WQUw7DQo+ICAJCWdvdG8gb3V0
Ow0KPiAgCX0NCj4gQEAgLTc2OSw3ICs3NzAsOCBAQCBzdGF0aWMgaW50IHgyNV9jb25uZWN0KHN0
cnVjdCBzb2NrZXQgKnNvY2ssIHN0cnVjdA0KPiBzb2NrYWRkciAqdWFkZHIsDQo+IA0KPiAgCXJj
ID0gLUVJTlZBTDsNCj4gIAlpZiAoYWRkcl9sZW4gIT0gc2l6ZW9mKHN0cnVjdCBzb2NrYWRkcl94
MjUpIHx8DQo+IC0JICAgIGFkZHItPnN4MjVfZmFtaWx5ICE9IEFGX1gyNSkNCj4gKwkgICAgYWRk
ci0+c3gyNV9mYW1pbHkgIT0gQUZfWDI1IHx8DQo+ICsJICAgIHN0cm5sZW4oYWRkci0+c3gyNV9h
ZGRyLngyNV9hZGRyLCBYMjVfQUREUl9MRU4pID09DQo+IFgyNV9BRERSX0xFTikNCj4gIAkJZ290
byBvdXQ7DQo+IA0KPiAgCXJjID0gLUVORVRVTlJFQUNIOw0KPiAtLQ0KPiAyLjI5LjINCg0K

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

* Re: [PATCH net v2] net/x25: prevent a couple of overflows
  2020-12-01 15:15 ` [PATCH net v2] net/x25: prevent a couple of overflows Dan Carpenter
  2020-12-02  7:43   ` [PATCH net v2] net/x25: prevent a couple of overflows(Internet mail) kiyin(尹亮)
@ 2020-12-02  9:27   ` Martin Schiller
  2020-12-03  1:27     ` Jakub Kicinski
  1 sibling, 1 reply; 4+ messages in thread
From: Martin Schiller @ 2020-12-02  9:27 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: David S. Miller, Jakub Kicinski, linux-x25, netdev,
	Andrew Hendry, kiyin(尹亮),
	security, linux-distros, huntchen(陈阳),
	dannywang(王宇),
	kernel-janitors

On 2020-12-01 16:15, Dan Carpenter wrote:
> The .x25_addr[] address comes from the user and is not necessarily
> NUL terminated.  This leads to a couple problems.  The first problem is
> that the strlen() in x25_bind() can read beyond the end of the buffer.
> 
> The second problem is more subtle and could result in memory 
> corruption.
> The call tree is:
>   x25_connect()
>   --> x25_write_internal()
>       --> x25_addr_aton()
> 
> The .x25_addr[] buffers are copied to the "addresses" buffer from
> x25_write_internal() so it will lead to stack corruption.
> 
> Verify that the strings are NUL terminated and return -EINVAL if they
> are not.
> 
> Reported-by: "kiyin(尹亮)" <kiyin@tencent.com>
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> The first patch put a NUL terminator on the end of the string and this
> patch returns an error instead.  I don't have a strong preference, 
> which
> patch to go with.
> 
>  net/x25/af_x25.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/net/x25/af_x25.c b/net/x25/af_x25.c
> index 9232cdb42ad9..d41fffb2507b 100644
> --- a/net/x25/af_x25.c
> +++ b/net/x25/af_x25.c
> @@ -675,7 +675,8 @@ static int x25_bind(struct socket *sock, struct
> sockaddr *uaddr, int addr_len)
>  	int len, i, rc = 0;
> 
>  	if (addr_len != sizeof(struct sockaddr_x25) ||
> -	    addr->sx25_family != AF_X25) {
> +	    addr->sx25_family != AF_X25 ||
> +	    strnlen(addr->sx25_addr.x25_addr, X25_ADDR_LEN) = X25_ADDR_LEN) 
> {
>  		rc = -EINVAL;
>  		goto out;
>  	}
> @@ -769,7 +770,8 @@ static int x25_connect(struct socket *sock, struct
> sockaddr *uaddr,
> 
>  	rc = -EINVAL;
>  	if (addr_len != sizeof(struct sockaddr_x25) ||
> -	    addr->sx25_family != AF_X25)
> +	    addr->sx25_family != AF_X25 ||
> +	    strnlen(addr->sx25_addr.x25_addr, X25_ADDR_LEN) = X25_ADDR_LEN)
>  		goto out;
> 
>  	rc = -ENETUNREACH;

Acked-by: Martin Schiller <ms@dev.tdt.de>

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

* Re: [PATCH net v2] net/x25: prevent a couple of overflows
  2020-12-02  9:27   ` [PATCH net v2] net/x25: prevent a couple of overflows Martin Schiller
@ 2020-12-03  1:27     ` Jakub Kicinski
  0 siblings, 0 replies; 4+ messages in thread
From: Jakub Kicinski @ 2020-12-03  1:27 UTC (permalink / raw)
  To: Martin Schiller
  Cc: Dan Carpenter, David S. Miller, linux-x25, netdev, Andrew Hendry,
	kiyin( 尹亮) ,
	security, linux-distros, huntchen(陈阳) ,
	dannywang(王宇) ,
	kernel-janitors

On Wed, 02 Dec 2020 10:27:18 +0100 Martin Schiller wrote:
> On 2020-12-01 16:15, Dan Carpenter wrote:
> > The .x25_addr[] address comes from the user and is not necessarily
> > NUL terminated.  This leads to a couple problems.  The first problem is
> > that the strlen() in x25_bind() can read beyond the end of the buffer.
> > 
> > The second problem is more subtle and could result in memory 
> > corruption.
> > The call tree is:
> >   x25_connect()  
> >   --> x25_write_internal()
> >       --> x25_addr_aton()  
> > 
> > The .x25_addr[] buffers are copied to the "addresses" buffer from
> > x25_write_internal() so it will lead to stack corruption.
> > 
> > Verify that the strings are NUL terminated and return -EINVAL if they
> > are not.
> > 
> > Reported-by: "kiyin(尹亮)" <kiyin@tencent.com>
> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> 
> Acked-by: Martin Schiller <ms@dev.tdt.de>

Applied, thanks!

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

end of thread, other threads:[~2020-12-03  1:27 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <ecf3321f20cc4f6dcf02b5b73105da58@dev.tdt.de>
2020-12-01 15:15 ` [PATCH net v2] net/x25: prevent a couple of overflows Dan Carpenter
2020-12-02  7:43   ` [PATCH net v2] net/x25: prevent a couple of overflows(Internet mail) kiyin(尹亮)
2020-12-02  9:27   ` [PATCH net v2] net/x25: prevent a couple of overflows Martin Schiller
2020-12-03  1:27     ` Jakub Kicinski

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).