lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Message-ID: <20201026234010.GD1947033@gmail.com> Date: Mon, 26 Oct 2020 16:40:10 -0700 From: Eric Biggers <ebiggers@...nel.org> To: "Gustavo A. R. Silva" <gustavoars@...nel.org> Cc: linux-crypto@...r.kernel.org, Herbert Xu <herbert@...dor.apana.org.au>, syzkaller-bugs@...glegroups.com, linux-hardening@...r.kernel.org, linux-api@...r.kernel.org, linux-kernel@...r.kernel.org, Jann Horn <jannh@...gle.com>, Kees Cook <keescook@...omium.org>, Elena Petrova <lenaptr@...gle.com>, Vegard Nossum <vegard.nossum@...cle.com>, stable@...r.kernel.org, syzbot+92ead4eb8e26a26d465e@...kaller.appspotmail.com Subject: Re: [PATCH] crypto: af_alg - avoid undefined behavior accessing salg_name On Mon, Oct 26, 2020 at 06:10:59PM -0500, Gustavo A. R. Silva wrote: > On Mon, Oct 26, 2020 at 04:21:48PM -0500, Gustavo A. R. Silva wrote: > > > +/* > > > + * Linux v4.12 and later removed the 64-byte limit on salg_name[]; it's now an > > > + * arbitrary-length field. We had to keep the original struct above for source > > > + * compatibility with existing userspace programs, though. Use the new struct > > > + * below if support for very long algorithm names is needed. To do this, > > > + * allocate 'sizeof(struct sockaddr_alg_new) + strlen(algname) + 1' bytes, and > > > + * copy algname (including the null terminator) into salg_name. > > > + */ > > > +struct sockaddr_alg_new { > > > + __u16 salg_family; > > > + __u8 salg_type[14]; > > > + __u32 salg_feat; > > > + __u32 salg_mask; > > > + __u8 salg_name[]; > > > +}; > > > + > > > > How something like this, instead: > > > > struct sockaddr_alg { > > - __u16 salg_family; > > - __u8 salg_type[14]; > > - __u32 salg_feat; > > - __u32 salg_mask; > > - __u8 salg_name[64]; > > + union { > > + struct { > > + __u16 salg_v1_family; > > + __u8 salg_v1_type[14]; > > + __u32 salg_v1_feat; > > + __u32 salg_v1_mask; > > + __u8 salg_name[64]; > > + }; > > + struct { > > + __u16 salg_family; > > + __u8 salg_type[14]; > > + __u32 salg_feat; > > + __u32 salg_mask; > > + __u8 salg_name_new[]; > > + }; > > + }; > > }; > > > > Something similar to the following approach might work: > > https://git.kernel.org/pub/scm/linux/kernel/git/gustavoars/linux.git/commit/?h=testing/uapi/gntalloc&id=db46c8aba41c436edb0b4ef2941bd7390b0e5d61 > I suppose so. It's very confusing to see a union like that at first glance, though. It definitely needs an explanatory comment... - Eric
Powered by blists - more mailing lists