Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751793AbdIOU0y (ORCPT ); Fri, 15 Sep 2017 16:26:54 -0400 Received: from youngberry.canonical.com ([91.189.89.112]:58914 "EHLO youngberry.canonical.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751620AbdIOU0x (ORCPT ); Fri, 15 Sep 2017 16:26:53 -0400 Date: Fri, 15 Sep 2017 13:26:41 -0700 From: Seth Arnold To: Arnd Bergmann Cc: John Johansen , James Morris , "Serge E. Hallyn" , Kees Cook , Stephen Rothwell , Michal Hocko , Vlastimil Babka , linux-security-module@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH] apparmor: initialized returned struct aa_perms Message-ID: <20170915202641.GC16809@hunt> References: <20170915195620.1561044-1-arnd@arndb.de> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="p2kqVDKq5asng8Dg" Content-Disposition: inline In-Reply-To: <20170915195620.1561044-1-arnd@arndb.de> User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5420 Lines: 142 --p2kqVDKq5asng8Dg Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Fri, Sep 15, 2017 at 09:55:46PM +0200, Arnd Bergmann wrote: > gcc-4.4 points out suspicious code in compute_mnt_perms, where > the aa_perms structure is only partially initialized before getting > returned: >=20 > security/apparmor/mount.c: In function 'compute_mnt_perms': > security/apparmor/mount.c:227: error: 'perms.prompt' is used uninitialize= d in this function > security/apparmor/mount.c:227: error: 'perms.hide' is used uninitialized = in this function > security/apparmor/mount.c:227: error: 'perms.cond' is used uninitialized = in this function > security/apparmor/mount.c:227: error: 'perms.complain' is used uninitiali= zed in this function > security/apparmor/mount.c:227: error: 'perms.stop' is used uninitialized = in this function > security/apparmor/mount.c:227: error: 'perms.deny' is used uninitialized = in this function >=20 > Returning or assigning partially initialized structures is a bit tricky, > in particular it is explicitly allowed in c99 to assign a partially > intialized structure to another, as long as only members are read that > have been initialized earlier. Looking at what various compilers do here, > the version that produced the warning copied unintialized stack data, > while newer versions (and also clang) either set the other members to > zero or don't update the parts of the return buffer that are not modified > in the temporary structure, but they never warn about this. >=20 > In case of apparmor, it seems better to be a little safer and always > initialize the aa_perms structure. Most users already do that, this > changes the remaining ones, including the one instance that I got the > warning for. >=20 > Fixes: fa488437d0f9 ("apparmor: add mount mediation") > Signed-off-by: Arnd Bergmann Reviewed-by: Seth Arnold Thanks > --- > security/apparmor/file.c | 8 +------- > security/apparmor/lib.c | 13 +++++-------- > security/apparmor/mount.c | 13 ++++++------- > 3 files changed, 12 insertions(+), 22 deletions(-) >=20 > diff --git a/security/apparmor/file.c b/security/apparmor/file.c > index db80221891c6..86d57e56fabe 100644 > --- a/security/apparmor/file.c > +++ b/security/apparmor/file.c > @@ -227,18 +227,12 @@ static u32 map_old_perms(u32 old) > struct aa_perms aa_compute_fperms(struct aa_dfa *dfa, unsigned int state, > struct path_cond *cond) > { > - struct aa_perms perms; > - > /* FIXME: change over to new dfa format > * currently file perms are encoded in the dfa, new format > * splits the permissions from the dfa. This mapping can be > * done at profile load > */ > - perms.deny =3D 0; > - perms.kill =3D perms.stop =3D 0; > - perms.complain =3D perms.cond =3D 0; > - perms.hide =3D 0; > - perms.prompt =3D 0; > + struct aa_perms perms =3D { }; > =20 > if (uid_eq(current_fsuid(), cond->uid)) { > perms.allow =3D map_old_perms(dfa_user_allow(dfa, state)); > diff --git a/security/apparmor/lib.c b/security/apparmor/lib.c > index 8818621b5d95..6cbc06da964c 100644 > --- a/security/apparmor/lib.c > +++ b/security/apparmor/lib.c > @@ -318,14 +318,11 @@ static u32 map_other(u32 x) > void aa_compute_perms(struct aa_dfa *dfa, unsigned int state, > struct aa_perms *perms) > { > - perms->deny =3D 0; > - perms->kill =3D perms->stop =3D 0; > - perms->complain =3D perms->cond =3D 0; > - perms->hide =3D 0; > - perms->prompt =3D 0; > - perms->allow =3D dfa_user_allow(dfa, state); > - perms->audit =3D dfa_user_audit(dfa, state); > - perms->quiet =3D dfa_user_quiet(dfa, state); > + *perms =3D (struct aa_perms) { > + .allow =3D dfa_user_allow(dfa, state), > + .audit =3D dfa_user_audit(dfa, state), > + .quiet =3D dfa_user_quiet(dfa, state), > + }; > =20 > /* for v5 perm mapping in the policydb, the other set is used > * to extend the general perm set > diff --git a/security/apparmor/mount.c b/security/apparmor/mount.c > index 82a64b58041d..ed9b4d0f9f7e 100644 > --- a/security/apparmor/mount.c > +++ b/security/apparmor/mount.c > @@ -216,13 +216,12 @@ static unsigned int match_mnt_flags(struct aa_dfa *= dfa, unsigned int state, > static struct aa_perms compute_mnt_perms(struct aa_dfa *dfa, > unsigned int state) > { > - struct aa_perms perms; > - > - perms.kill =3D 0; > - perms.allow =3D dfa_user_allow(dfa, state); > - perms.audit =3D dfa_user_audit(dfa, state); > - perms.quiet =3D dfa_user_quiet(dfa, state); > - perms.xindex =3D dfa_user_xindex(dfa, state); > + struct aa_perms perms =3D { > + .allow =3D dfa_user_allow(dfa, state), > + .audit =3D dfa_user_audit(dfa, state), > + .quiet =3D dfa_user_quiet(dfa, state), > + .xindex =3D dfa_user_xindex(dfa, state), > + }; > =20 > return perms; > } --p2kqVDKq5asng8Dg Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQEcBAEBCgAGBQJZvDeBAAoJEPMhclmdjS6XL30H+gOQOYaaGpn1aivJepf7SV44 PEyCEyib4v5oovdgC4jw2czEKGCErWP0k+dFYgyCkrJKL9IQgL8mgAK8CvP6tpb9 6TYhSthEe9YLv6q5QwYD+DtPLH3Sdls8A7KlieXLaQuwkJFBm0k6mSGc7J2tC+lj HFDuYNrWXhPgFcGyJyJHqCjH8nT0AdbwjDmHagCjnpscpeVEb+L0c+LWJFqNZY0i eruuYRn7OeFX0ZRDVMAFqYZkVGctFTalsZVuFKAaDpOsAOZenWsAzryMRTdxj+fT 5+pWwsUks97c4gZoOo7ZP9AVn86wRfh4bREsBzZkI6zbEqU6rfNqvgqRmSuBYZ8= =EPRE -----END PGP SIGNATURE----- --p2kqVDKq5asng8Dg--