linux-hardening - Re: [PATCH] crypto: af_alg - avoid undefined behavior accessing salg

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

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