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