Re: [PATCH] crypto: af_alg - avoid undefined behavior accessing salg_name

From: Eric Biggers
Date: Mon Oct 26 2020 - 19:40:14 EST


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