* Re: [PATCH] apparmor: initialized returned struct aa_perms
2017-09-15 19:55 [PATCH] apparmor: initialized returned struct aa_perms Arnd Bergmann
@ 2017-09-15 20:26 ` Seth Arnold
2017-09-25 11:29 ` Geert Uytterhoeven
2017-09-25 14:29 ` John Johansen
2 siblings, 0 replies; 7+ messages in thread
From: Seth Arnold @ 2017-09-15 20:26 UTC (permalink / raw)
To: Arnd Bergmann
Cc: John Johansen, James Morris, Serge E. Hallyn, Kees Cook,
Stephen Rothwell, Michal Hocko, Vlastimil Babka,
linux-security-module, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 4722 bytes --]
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:
>
> security/apparmor/mount.c: In function 'compute_mnt_perms':
> security/apparmor/mount.c:227: error: 'perms.prompt' is used uninitialized 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 uninitialized 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
>
> 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.
>
> 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.
>
> Fixes: fa488437d0f9 ("apparmor: add mount mediation")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Reviewed-by: Seth Arnold <seth.arnold@canonical.com>
Thanks
> ---
> security/apparmor/file.c | 8 +-------
> security/apparmor/lib.c | 13 +++++--------
> security/apparmor/mount.c | 13 ++++++-------
> 3 files changed, 12 insertions(+), 22 deletions(-)
>
> 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 = 0;
> - perms.kill = perms.stop = 0;
> - perms.complain = perms.cond = 0;
> - perms.hide = 0;
> - perms.prompt = 0;
> + struct aa_perms perms = { };
>
> if (uid_eq(current_fsuid(), cond->uid)) {
> perms.allow = 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 = 0;
> - perms->kill = perms->stop = 0;
> - perms->complain = perms->cond = 0;
> - perms->hide = 0;
> - perms->prompt = 0;
> - perms->allow = dfa_user_allow(dfa, state);
> - perms->audit = dfa_user_audit(dfa, state);
> - perms->quiet = dfa_user_quiet(dfa, state);
> + *perms = (struct aa_perms) {
> + .allow = dfa_user_allow(dfa, state),
> + .audit = dfa_user_audit(dfa, state),
> + .quiet = dfa_user_quiet(dfa, state),
> + };
>
> /* 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 = 0;
> - perms.allow = dfa_user_allow(dfa, state);
> - perms.audit = dfa_user_audit(dfa, state);
> - perms.quiet = dfa_user_quiet(dfa, state);
> - perms.xindex = dfa_user_xindex(dfa, state);
> + struct aa_perms perms = {
> + .allow = dfa_user_allow(dfa, state),
> + .audit = dfa_user_audit(dfa, state),
> + .quiet = dfa_user_quiet(dfa, state),
> + .xindex = dfa_user_xindex(dfa, state),
> + };
>
> return perms;
> }
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] apparmor: initialized returned struct aa_perms
2017-09-15 19:55 [PATCH] apparmor: initialized returned struct aa_perms Arnd Bergmann
2017-09-15 20:26 ` Seth Arnold
@ 2017-09-25 11:29 ` Geert Uytterhoeven
2017-09-25 14:29 ` John Johansen
2 siblings, 0 replies; 7+ messages in thread
From: Geert Uytterhoeven @ 2017-09-25 11:29 UTC (permalink / raw)
To: Arnd Bergmann
Cc: John Johansen, James Morris, Serge E. Hallyn, Kees Cook,
Stephen Rothwell, Seth Arnold, Michal Hocko, Vlastimil Babka,
linux-security-module, linux-kernel@vger.kernel.org
On Fri, Sep 15, 2017 at 9:55 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> gcc-4.4 points out suspicious code in compute_mnt_perms, where
> the aa_perms structure is only partially initialized before getting
> returned:
>
> security/apparmor/mount.c: In function 'compute_mnt_perms':
> security/apparmor/mount.c:227: error: 'perms.prompt' is used uninitialized 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 uninitialized 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
>
> 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.
>
> 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.
>
> Fixes: fa488437d0f9 ("apparmor: add mount mediation")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Acked-by: Geert Uytterhoeven <geert@linux-m68k.org>
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] apparmor: initialized returned struct aa_perms
2017-09-15 19:55 [PATCH] apparmor: initialized returned struct aa_perms Arnd Bergmann
2017-09-15 20:26 ` Seth Arnold
2017-09-25 11:29 ` Geert Uytterhoeven
@ 2017-09-25 14:29 ` John Johansen
2017-11-20 14:00 ` Arnd Bergmann
2 siblings, 1 reply; 7+ messages in thread
From: John Johansen @ 2017-09-25 14:29 UTC (permalink / raw)
To: Arnd Bergmann, James Morris, Serge E. Hallyn
Cc: Kees Cook, Stephen Rothwell, Seth Arnold, Michal Hocko,
Vlastimil Babka, linux-security-module, linux-kernel
On 09/15/2017 03:55 PM, 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:
>
> security/apparmor/mount.c: In function 'compute_mnt_perms':
> security/apparmor/mount.c:227: error: 'perms.prompt' is used uninitialized 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 uninitialized 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
>
> 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.
>
> 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.
>
> Fixes: fa488437d0f9 ("apparmor: add mount mediation")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
I've pulled this into apparmor-next
> ---
> security/apparmor/file.c | 8 +-------
> security/apparmor/lib.c | 13 +++++--------
> security/apparmor/mount.c | 13 ++++++-------
> 3 files changed, 12 insertions(+), 22 deletions(-)
>
> 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 = 0;
> - perms.kill = perms.stop = 0;
> - perms.complain = perms.cond = 0;
> - perms.hide = 0;
> - perms.prompt = 0;
> + struct aa_perms perms = { };
>
> if (uid_eq(current_fsuid(), cond->uid)) {
> perms.allow = 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 = 0;
> - perms->kill = perms->stop = 0;
> - perms->complain = perms->cond = 0;
> - perms->hide = 0;
> - perms->prompt = 0;
> - perms->allow = dfa_user_allow(dfa, state);
> - perms->audit = dfa_user_audit(dfa, state);
> - perms->quiet = dfa_user_quiet(dfa, state);
> + *perms = (struct aa_perms) {
> + .allow = dfa_user_allow(dfa, state),
> + .audit = dfa_user_audit(dfa, state),
> + .quiet = dfa_user_quiet(dfa, state),
> + };
>
> /* 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 = 0;
> - perms.allow = dfa_user_allow(dfa, state);
> - perms.audit = dfa_user_audit(dfa, state);
> - perms.quiet = dfa_user_quiet(dfa, state);
> - perms.xindex = dfa_user_xindex(dfa, state);
> + struct aa_perms perms = {
> + .allow = dfa_user_allow(dfa, state),
> + .audit = dfa_user_audit(dfa, state),
> + .quiet = dfa_user_quiet(dfa, state),
> + .xindex = dfa_user_xindex(dfa, state),
> + };
>
> return perms;
> }
>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] apparmor: initialized returned struct aa_perms
2017-09-25 14:29 ` John Johansen
@ 2017-11-20 14:00 ` Arnd Bergmann
2017-11-20 15:47 ` John Johansen
0 siblings, 1 reply; 7+ messages in thread
From: Arnd Bergmann @ 2017-11-20 14:00 UTC (permalink / raw)
To: John Johansen
Cc: James Morris, Serge E. Hallyn, Kees Cook, Stephen Rothwell,
Seth Arnold, Michal Hocko, Vlastimil Babka, LSM List,
Linux Kernel Mailing List
On Mon, Sep 25, 2017 at 4:29 PM, John Johansen
<john.johansen@canonical.com> wrote:
> On 09/15/2017 03:55 PM, 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:
>>
>> security/apparmor/mount.c: In function 'compute_mnt_perms':
>> security/apparmor/mount.c:227: error: 'perms.prompt' is used uninitialized 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 uninitialized 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
>>
>> 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.
>>
>> 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.
>>
>> Fixes: fa488437d0f9 ("apparmor: add mount mediation")
>> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>
> I've pulled this into apparmor-next
It apparently never made it into mainline. What happened?
Arnd
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] apparmor: initialized returned struct aa_perms
2017-11-20 14:00 ` Arnd Bergmann
@ 2017-11-20 15:47 ` John Johansen
2017-11-20 16:05 ` Arnd Bergmann
0 siblings, 1 reply; 7+ messages in thread
From: John Johansen @ 2017-11-20 15:47 UTC (permalink / raw)
To: Arnd Bergmann
Cc: James Morris, Serge E. Hallyn, Kees Cook, Stephen Rothwell,
Seth Arnold, Michal Hocko, Vlastimil Babka, LSM List,
Linux Kernel Mailing List
On 11/20/2017 06:00 AM, Arnd Bergmann wrote:
> On Mon, Sep 25, 2017 at 4:29 PM, John Johansen
> <john.johansen@canonical.com> wrote:
>> On 09/15/2017 03:55 PM, 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:
>>>
>>> security/apparmor/mount.c: In function 'compute_mnt_perms':
>>> security/apparmor/mount.c:227: error: 'perms.prompt' is used uninitialized 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 uninitialized 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
>>>
>>> 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.
>>>
>>> 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.
>>>
>>> Fixes: fa488437d0f9 ("apparmor: add mount mediation")
>>> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>>
>> I've pulled this into apparmor-next
>
> It apparently never made it into mainline. What happened?
>
> Arnd
>
Its in apparmor-next and is going with today's pull request
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] apparmor: initialized returned struct aa_perms
2017-11-20 15:47 ` John Johansen
@ 2017-11-20 16:05 ` Arnd Bergmann
0 siblings, 0 replies; 7+ messages in thread
From: Arnd Bergmann @ 2017-11-20 16:05 UTC (permalink / raw)
To: John Johansen
Cc: James Morris, Serge E. Hallyn, Kees Cook, Stephen Rothwell,
Seth Arnold, Michal Hocko, Vlastimil Babka, LSM List,
Linux Kernel Mailing List
On Mon, Nov 20, 2017 at 4:47 PM, John Johansen
<john.johansen@canonical.com> wrote:
> On 11/20/2017 06:00 AM, Arnd Bergmann wrote:
>> On Mon, Sep 25, 2017 at 4:29 PM, John Johansen
>> <john.johansen@canonical.com> wrote:
>>> On 09/15/2017 03:55 PM, 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:
>>>>
>>>> security/apparmor/mount.c: In function 'compute_mnt_perms':
>>>> security/apparmor/mount.c:227: error: 'perms.prompt' is used uninitialized 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 uninitialized 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
>>>>
>>>> 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.
>>>>
>>>> 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.
>>>>
>>>> Fixes: fa488437d0f9 ("apparmor: add mount mediation")
>>>> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>>>
>>> I've pulled this into apparmor-next
>>
>> It apparently never made it into mainline. What happened?
>>
> Its in apparmor-next and is going with today's pull request
Ok, thanks for checking. I see it in linux-next now, but didn't see
it a linux-next tree from early last week, or in mainline.
Arnd
^ permalink raw reply [flat|nested] 7+ messages in thread