Topics

Friday is for fun: fixing the error paths in selinux


Lukas Bulwahn
 

Dear all,

today, the Tool Investigation and Code Improvement Group (a.k.a. Fun &
Happiness Group) is making an offer to fix a bug in the kernel for
free and with fun:

security/selinux/ss/services.c:2039:2: warning: Value stored to 'rc'
is never read [clang-analyzer-deadcode.DeadStores]
rc = -EINVAL;
^
security/selinux/ss/services.c:2048:2: warning: Value stored to 'rc'
is never read [clang-analyzer-deadcode.DeadStores]
rc = -EINVAL;
^
security/selinux/ss/services.c:2056:2: warning: Value stored to 'rc'
is never read [clang-analyzer-deadcode.DeadStores]
rc = -EINVAL;
^
security/selinux/ss/services.c:2080:3: warning: Value stored to 'rc'
is never read [clang-analyzer-deadcode.DeadStores]
rc = -EINVAL;
^

on next-20201203.

Look at that function and you will see that the rc is overwritten in
the error path named bad. That seems to be unintentional; so, the task
is to understand how we got into the mess and what the actual
intention of the various authors was and where that just went wrong...

Once that is understood, you are probably ready to prepare a patch to
submit to linux-safety and explain your fix...

In case, you pick it and need some help, just reach out on the mailing
list... we are always happy to help...

Get your first kernel patch ready and enjoy your Friday and your weekend,

If it is too easy for you, we have some more difficult bugs at hand :)

Lukas


Jochen Kall
 

Hi Lukas, everyone,

this looks fun indeed, so I gave it a try yesterday night.
Nice challenge, it is shaping up to be a good excample for why stuff is not that simple and streightforward to trace, even with Git at your disposal, but I guess you knew that already ^^

Not sure if got to the bottom of this mystery, but maybe something can be learned from the path I took to get there and from what more experienced Git users have to say about it.

What I did and what I learned:
1) git blame -L 2035,2045 -l services.c
- Gives us the revision where the dead stores were introduced in the first place
4b02b524487622ce1cf472123899520b583f47dc
Author is Eric Paris <eparis@redhat.com>
- The log entry states, this commit is to unify the design pattern of the error paths to the following pattern:
rc=errno
if (failure)
goto out;
[...]
out:
cleanup()
return rc;
- So the idea was to first set the errorcode rc, and jump to "out:" in case of a failure, where the errorcode rc would be returned (Possibly another example of the one return per function only design paradigm)
- Nevetheless in that commit the dead stores are already introduced in all instances where the control jumps to the label "bad:" instead of "out:"
Quite possibly on purpose though to have the same pattern everywhere and possibly to protect against introduction of future bugs popping up when the code might be modified to jump to "out:" instead.
2) I assume, the code was reworked at a later date doing away with the pattern Eric used, so I looked at the revision removing the label "out:", after all it is not in the most recent version of code anymore
git blame --reverse 4b02b524487622ce1cf472123899520b583f47dc.. -l services.c
- That way I find the last revision still having the line 7d0250ed8e69fb6a66caecf60b8753a21224cc1a which means the next revision 23d19ba06b9c5614d6457f5fed349ec8f6d4dac9 is the one making the change.
3) Turns out this is a merge revision merging a branch, that itself came from Linux 5.0-rc1 with hash bfeffd155283772bbe78c6a05dec7c0128ee500c
4) Another Blame like 1) to find where that version comes from leads right back to Eric commit above, so it found it's way into Linux 5.0-rc1 as well at some point.
- This means the messy situation basically boils down to having the same patch integrated in two parallel branches that then are merged at some point discarding the change, which leads to blame leading us down the wrong branch so to speak.
5) This also means the changes must have happend somehwere between Erics commit and the head of Linux 5.0-rc1.
-To get the chain of commits between Erics commit and the release candidate 5.0-rc1 I tried to use the --ancestry path option of git rev-list
git rev-list --ancestry-path 4b02b524487622ce1cf472123899520b583f47dc..bfeffd155283772bbe78c6a05dec7c0128ee500c
- Maybe I misunderstand how this command works, but according to the output, there are 576463 commits on the path between the Erics Commit and the head of 5.0-rc1 ... so thats a dead end.
6) One more attempt with a different vector of attack, I set a view in gitk to filter all commits making changes to services.c and sure enough a "manual" bisection yields the revision removing the "output:" label
ee1a84fdfeedfd7362e9a8a8f15fedc3482ade2d by Ondrej Mosnacek <omosnace@redhat.com> 2018-11-30 16:24:08
- This seems to be a functional change of how stuff works under the hood, and the removal of the "output:" label is not adressed in the log entry, seems to be a byproduct of reworking the function.
- In that revision all instances where before there would be a jump to "out:" are replaced by direct returns, and the preceding setting of the variable "rc" is removed, the other instances however are not touched and remain in there.

My conclusion:
Most likely a remnant leftover from Ondrej changing the defensive programming design pattern Eric introduced a decade earlier.

What do you think?

Jochen

-----Ursprüngliche Nachricht-----
Von: devel@lists.elisa.tech <devel@lists.elisa.tech> Im Auftrag von Lukas
Bulwahn
Gesendet: Freitag, 4. Dezember 2020 11:35
An: devel@lists.elisa.tech
Betreff: [ELISA Technical Community] Friday is for fun: fixing the error paths
in selinux

Dear all,

today, the Tool Investigation and Code Improvement Group (a.k.a. Fun &
Happiness Group) is making an offer to fix a bug in the kernel for free and
with fun:

security/selinux/ss/services.c:2039:2: warning: Value stored to 'rc'
is never read [clang-analyzer-deadcode.DeadStores]
rc = -EINVAL;
^
security/selinux/ss/services.c:2048:2: warning: Value stored to 'rc'
is never read [clang-analyzer-deadcode.DeadStores]
rc = -EINVAL;
^
security/selinux/ss/services.c:2056:2: warning: Value stored to 'rc'
is never read [clang-analyzer-deadcode.DeadStores]
rc = -EINVAL;
^
security/selinux/ss/services.c:2080:3: warning: Value stored to 'rc'
is never read [clang-analyzer-deadcode.DeadStores]
rc = -EINVAL;
^

on next-20201203.

Look at that function and you will see that the rc is overwritten in the error
path named bad. That seems to be unintentional; so, the task is to
understand how we got into the mess and what the actual intention of the
various authors was and where that just went wrong...

Once that is understood, you are probably ready to prepare a patch to
submit to linux-safety and explain your fix...

In case, you pick it and need some help, just reach out on the mailing list...
we are always happy to help...

Get your first kernel patch ready and enjoy your Friday and your weekend,

If it is too easy for you, we have some more difficult bugs at hand :)

Lukas



--
Mit freundlichen Grüßen
Jochen Kall

--
Dr. rer. nat. Jochen Kall

Funktionale Sicherheit

ITK Engineering GmbH
Im Speyerer Tal 6
76761 Rülzheim

Tel.: +49 7272 7703-546
Fax: +49 7272 7703-100

Mobil:+491734957776

mailto:jochen.kall@itk-engineering.de ( jochen.kall@itk-engineering.de )

______________________________________________________________

ITK Engineering GmbH | Im Speyerer Tal 6 | 76761 Rülzheim

Tel.: +49 7272 7703-0 | Fax: +49 7272 7703-100

mailto:info@itk-engineering.de ( info@itk-engineering.de ) | http://www.itk-engineering.de ( http://www.itk-engineering.de/ )

Vorsitzender des Aufsichtsrats/Chairman of the Supervisory Board:

Dr. Rudolf Maier

Geschäftsführung/Executive Board:

Michael Englert (Vorsitzender/Chairman), Bernd Gohlicke

Sitz der Gesellschaft/Registered Office: 76761 Rülzheim

Registergericht/Registered Court: Amtsgericht Landau, HRB 32046

USt.-ID-Nr./VAT-ID-No. DE 813165046


Lukas Bulwahn
 

On Mon, Dec 7, 2020 at 10:15 AM Jochen Kall
<Jochen.Kall@itk-engineering.de> wrote:

Hi Lukas, everyone,

this looks fun indeed, so I gave it a try yesterday night.
Nice challenge, it is shaping up to be a good excample for why stuff is not that simple and streightforward to trace, even with Git at your disposal, but I guess you knew that already ^^
Yes, git blame is the right tool; in fact, I use tig blame (which has
a slightly easier user interface), but it essentially comes with the
same functionality.

Not sure if got to the bottom of this mystery, but maybe something can be learned from the path I took to get there and from what more experienced Git users have to say about it.

What I did and what I learned:
1) git blame -L 2035,2045 -l services.c
- Gives us the revision where the dead stores were introduced in the first place
4b02b524487622ce1cf472123899520b583f47dc
Author is Eric Paris <eparis@redhat.com>
- The log entry states, this commit is to unify the design pattern of the error paths to the following pattern:
rc=errno
if (failure)
goto out;
[...]
out:
cleanup()
return rc;
- So the idea was to first set the errorcode rc, and jump to "out:" in case of a failure, where the errorcode rc would be returned (Possibly another example of the one return per function only design paradigm)
- Nevetheless in that commit the dead stores are already introduced in all instances where the control jumps to the label "bad:" instead of "out:"
Quite possibly on purpose though to have the same pattern everywhere and possibly to protect against introduction of future bugs popping up when the code might be modified to jump to "out:" instead.
Yes, those are the general code patterns in the kernel. As Dykstra
clearly stated: "goto is good!" when used for the right purpose!
(Unfortunately, another quote is remembered instead)

2) I assume, the code was reworked at a later date doing away with the pattern Eric used, so I looked at the revision removing the label "out:", after all it is not in the most recent version of code anymore
git blame --reverse 4b02b524487622ce1cf472123899520b583f47dc.. -l services.c
- That way I find the last revision still having the line 7d0250ed8e69fb6a66caecf60b8753a21224cc1a which means the next revision 23d19ba06b9c5614d6457f5fed349ec8f6d4dac9 is the one making the change.
Interesting, I did not know about that functionality with --reverse. I
need to try that out, when I come across another investigation.

3) Turns out this is a merge revision merging a branch, that itself came from Linux 5.0-rc1 with hash bfeffd155283772bbe78c6a05dec7c0128ee500c
4) Another Blame like 1) to find where that version comes from leads right back to Eric commit above, so it found it's way into Linux 5.0-rc1 as well at some point.
- This means the messy situation basically boils down to having the same patch integrated in two parallel branches that then are merged at some point discarding the change, which leads to blame leading us down the wrong branch so to speak.
Autsch... so maybe: too many people stepping on each others feet; no
wonder that code looks so stupid... probably nobody ever reviewed it
once it was merged in this broken state?

5) This also means the changes must have happend somehwere between Erics commit and the head of Linux 5.0-rc1.
-To get the chain of commits between Erics commit and the release candidate 5.0-rc1 I tried to use the --ancestry path option of git rev-list
git rev-list --ancestry-path 4b02b524487622ce1cf472123899520b583f47dc..bfeffd155283772bbe78c6a05dec7c0128ee500c
- Maybe I misunderstand how this command works, but according to the output, there are 576463 commits on the path between the Erics Commit and the head of 5.0-rc1 ... so thats a dead end.
I think you got something wrong here... maybe someone else can try and
help. I think you can boil it down to a smaller set of changes.

6) One more attempt with a different vector of attack, I set a view in gitk to filter all commits making changes to services.c and sure enough a "manual" bisection yields the revision removing the "output:" label
ee1a84fdfeedfd7362e9a8a8f15fedc3482ade2d by Ondrej Mosnacek <omosnace@redhat.com> 2018-11-30 16:24:08
- This seems to be a functional change of how stuff works under the hood, and the removal of the "output:" label is not adressed in the log entry, seems to be a byproduct of reworking the function.
- In that revision all instances where before there would be a jump to "out:" are replaced by direct returns, and the preceding setting of the variable "rc" is removed, the other instances however are not touched and remain in there.

My conclusion:
Most likely a remnant leftover from Ondrej changing the defensive programming design pattern Eric introduced a decade earlier.

What do you think?
I suggest we give others a few days and their own chance to investigate...

I did not do the investigation yet :) but you caught my attention now as well :)

I will follow up with my own investigation in the next few days if
nobody else does. Let us see what I will learn about this code...

And of course, it is Monday and we are all already looking forward to
a new Friday coming up... so I am also already looking for the next
fun exercise...

Lukas


Milan Lakhani
 

Hi,

Thanks for sharing this challenge Lukas, and your nice attack Jochen. I was on holiday when it was set but have looked at it, it's definitely an interesting case. For step 5 I'm wondering if, instead of using --ancestry, we could focus on the commits in one branch - maybe the drm/drm-next branch that was mentioned in the 23d19ba06b commit, where the 'out' labels that Jochen mentions were removed?

Speak soon,

Milan

On 07/12/2020 10:41, Lukas Bulwahn wrote:
On Mon, Dec 7, 2020 at 10:15 AM Jochen Kall
<Jochen.Kall@itk-engineering.de> wrote:
Hi Lukas, everyone,

this looks fun indeed, so I gave it a try yesterday night.
Nice challenge, it is shaping up to be a good excample for why stuff is not that simple and streightforward to trace, even with Git at your disposal, but I guess you knew that already ^^
Yes, git blame is the right tool; in fact, I use tig blame (which has
a slightly easier user interface), but it essentially comes with the
same functionality.

Not sure if got to the bottom of this mystery, but maybe something can be learned from the path I took to get there and from what more experienced Git users have to say about it.

What I did and what I learned:
1) git blame -L 2035,2045 -l services.c
- Gives us the revision where the dead stores were introduced in the first place
4b02b524487622ce1cf472123899520b583f47dc
Author is Eric Paris <eparis@redhat.com>
- The log entry states, this commit is to unify the design pattern of the error paths to the following pattern:
rc=errno
if (failure)
goto out;
[...]
out:
cleanup()
return rc;
- So the idea was to first set the errorcode rc, and jump to "out:" in case of a failure, where the errorcode rc would be returned (Possibly another example of the one return per function only design paradigm)
- Nevetheless in that commit the dead stores are already introduced in all instances where the control jumps to the label "bad:" instead of "out:"
Quite possibly on purpose though to have the same pattern everywhere and possibly to protect against introduction of future bugs popping up when the code might be modified to jump to "out:" instead.
Yes, those are the general code patterns in the kernel. As Dykstra
clearly stated: "goto is good!" when used for the right purpose!
(Unfortunately, another quote is remembered instead)

2) I assume, the code was reworked at a later date doing away with the pattern Eric used, so I looked at the revision removing the label "out:", after all it is not in the most recent version of code anymore
git blame --reverse 4b02b524487622ce1cf472123899520b583f47dc.. -l services.c
- That way I find the last revision still having the line 7d0250ed8e69fb6a66caecf60b8753a21224cc1a which means the next revision 23d19ba06b9c5614d6457f5fed349ec8f6d4dac9 is the one making the change.
Interesting, I did not know about that functionality with --reverse. I
need to try that out, when I come across another investigation.

3) Turns out this is a merge revision merging a branch, that itself came from Linux 5.0-rc1 with hash bfeffd155283772bbe78c6a05dec7c0128ee500c
4) Another Blame like 1) to find where that version comes from leads right back to Eric commit above, so it found it's way into Linux 5.0-rc1 as well at some point.
- This means the messy situation basically boils down to having the same patch integrated in two parallel branches that then are merged at some point discarding the change, which leads to blame leading us down the wrong branch so to speak.
Autsch... so maybe: too many people stepping on each others feet; no
wonder that code looks so stupid... probably nobody ever reviewed it
once it was merged in this broken state?

5) This also means the changes must have happend somehwere between Erics commit and the head of Linux 5.0-rc1.
-To get the chain of commits between Erics commit and the release candidate 5.0-rc1 I tried to use the --ancestry path option of git rev-list
git rev-list --ancestry-path 4b02b524487622ce1cf472123899520b583f47dc..bfeffd155283772bbe78c6a05dec7c0128ee500c
- Maybe I misunderstand how this command works, but according to the output, there are 576463 commits on the path between the Erics Commit and the head of 5.0-rc1 ... so thats a dead end.
I think you got something wrong here... maybe someone else can try and
help. I think you can boil it down to a smaller set of changes.

6) One more attempt with a different vector of attack, I set a view in gitk to filter all commits making changes to services.c and sure enough a "manual" bisection yields the revision removing the "output:" label
ee1a84fdfeedfd7362e9a8a8f15fedc3482ade2d by Ondrej Mosnacek <omosnace@redhat.com> 2018-11-30 16:24:08
- This seems to be a functional change of how stuff works under the hood, and the removal of the "output:" label is not adressed in the log entry, seems to be a byproduct of reworking the function.
- In that revision all instances where before there would be a jump to "out:" are replaced by direct returns, and the preceding setting of the variable "rc" is removed, the other instances however are not touched and remain in there.

My conclusion:
Most likely a remnant leftover from Ondrej changing the defensive programming design pattern Eric introduced a decade earlier.

What do you think?
I suggest we give others a few days and their own chance to investigate...

I did not do the investigation yet :) but you caught my attention now as well :)

I will follow up with my own investigation in the next few days if
nobody else does. Let us see what I will learn about this code...

And of course, it is Monday and we are all already looking forward to
a new Friday coming up... so I am also already looking for the next
fun exercise...

Lukas





Jochen Kall
 

Hi Milan,

sorry to answer this late, pre christmas crunch :/

git rev-list --ancestry-path turned out to be not the right tool for the job, since it collects all commits on all potential paths between the two commits. (should have read the manual more throroughly)
This means especially if we have a full development cycle with plenty of branching, ultimately being merged into the next mainline version, as it is the case in this example, the results don't get us anywhere.
It might be useful for figuring out similar questions as long as the commits are not as far apart though.

I'll have to look into this "manual bisection" again and see if I can substitute it with a git command, don't want to do that on a regular basis ^^

Don't remember drm/drm-next, might look into it during christmas downtime.

Best regards
Jochen

-----Ursprüngliche Nachricht-----
Von: devel@lists.elisa.tech <devel@lists.elisa.tech> Im Auftrag von Milan
Lakhani
Gesendet: Dienstag, 15. Dezember 2020 14:54
An: devel@lists.elisa.tech
Betreff: Re: [ELISA Technical Community] Friday is for fun: fixing the error
paths in selinux

Hi,

Thanks for sharing this challenge Lukas, and your nice attack Jochen. I was
on holiday when it was set but have looked at it, it's definitely an interesting
case. For step 5 I'm wondering if, instead of using --ancestry, we could focus
on the commits in one branch - maybe the drm/drm-next branch that was
mentioned in the 23d19ba06b commit, where the 'out' labels that Jochen
mentions were removed?

Speak soon,

Milan

On 07/12/2020 10:41, Lukas Bulwahn wrote:
On Mon, Dec 7, 2020 at 10:15 AM Jochen Kall
<Jochen.Kall@itk-engineering.de> wrote:
Hi Lukas, everyone,

this looks fun indeed, so I gave it a try yesterday night.
Nice challenge, it is shaping up to be a good excample for why stuff
is not that simple and streightforward to trace, even with Git at
your disposal, but I guess you knew that already ^^
Yes, git blame is the right tool; in fact, I use tig blame (which has
a slightly easier user interface), but it essentially comes with the
same functionality.

Not sure if got to the bottom of this mystery, but maybe something can
be learned from the path I took to get there and from what more
experienced Git users have to say about it.

What I did and what I learned:
1) git blame -L 2035,2045 -l services.c
- Gives us the revision where the dead stores were introduced in the
first place
4b02b524487622ce1cf472123899520b583f47dc
Author is Eric Paris <eparis@redhat.com>
- The log entry states, this commit is to unify the design pattern of
the error paths to the following pattern:
rc=errno
if (failure)
goto out;
[...]
out:
cleanup()
return rc;
- So the idea was to first set the errorcode rc, and jump to "out:" in
case of a failure, where the errorcode rc would be returned (Possibly another
example of the one return per function only design paradigm)
- Nevetheless in that commit the dead stores are already introduced
in all instances where the control jumps to the label "bad:" instead of "out:"
Quite possibly on purpose though to have the same pattern
everywhere and possibly to protect against introduction of future bugs
popping up when the code might be modified to jump to "out:" instead.
Yes, those are the general code patterns in the kernel. As Dykstra
clearly stated: "goto is good!" when used for the right purpose!
(Unfortunately, another quote is remembered instead)

2) I assume, the code was reworked at a later date doing away with the
pattern Eric used, so I looked at the revision removing the label "out:", after
all it is not in the most recent version of code anymore
git blame --reverse 4b02b524487622ce1cf472123899520b583f47dc..
-l services.c
- That way I find the last revision still having the line
7d0250ed8e69fb6a66caecf60b8753a21224cc1a which means the next
revision 23d19ba06b9c5614d6457f5fed349ec8f6d4dac9 is the one making
the change.
Interesting, I did not know about that functionality with --reverse. I
need to try that out, when I come across another investigation.

3) Turns out this is a merge revision merging a branch, that itself
came from Linux 5.0-rc1 with hash
bfeffd155283772bbe78c6a05dec7c0128ee500c
4) Another Blame like 1) to find where that version comes from leads right
back to Eric commit above, so it found it's way into Linux 5.0-rc1 as well at
some point.
- This means the messy situation basically boils down to having the
same patch integrated in two parallel branches that then are merged at
some point discarding the change, which leads to blame leading us down the
wrong branch so to speak.
Autsch... so maybe: too many people stepping on each others feet; no
wonder that code looks so stupid... probably nobody ever reviewed it
once it was merged in this broken state?

5) This also means the changes must have happend somehwere between
Erics commit and the head of Linux 5.0-rc1.
-To get the chain of commits between Erics commit and the release
candidate 5.0-rc1 I tried to use the --ancestry path option of git rev-list
git rev-list --ancestry-path
4b02b524487622ce1cf472123899520b583f47dc..bfeffd155283772bbe78c6a
05dec7c0128ee500c
- Maybe I misunderstand how this command works, but according to
the output, there are 576463 commits on the path between the Erics Commit
and the head of 5.0-rc1 ... so thats a dead end.
I think you got something wrong here... maybe someone else can try and
help. I think you can boil it down to a smaller set of changes.

6) One more attempt with a different vector of attack, I set a view in gitk
to filter all commits making changes to services.c and sure enough a
"manual" bisection yields the revision removing the "output:" label
ee1a84fdfeedfd7362e9a8a8f15fedc3482ade2d by Ondrej Mosnacek
<omosnace@redhat.com> 2018-11-30 16:24:08
- This seems to be a functional change of how stuff works under the
hood, and the removal of the "output:" label is not adressed in the log entry,
seems to be a byproduct of reworking the function.
- In that revision all instances where before there would be a jump to
"out:" are replaced by direct returns, and the preceding setting of the
variable "rc" is removed, the other instances however are not touched and
remain in there.

My conclusion:
Most likely a remnant leftover from Ondrej changing the defensive
programming design pattern Eric introduced a decade earlier.

What do you think?
I suggest we give others a few days and their own chance to investigate...

I did not do the investigation yet :) but you caught my attention now
as well :)

I will follow up with my own investigation in the next few days if
nobody else does. Let us see what I will learn about this code...

And of course, it is Monday and we are all already looking forward to
a new Friday coming up... so I am also already looking for the next
fun exercise...

Lukas







--
Mit freundlichen Grüßen
Jochen Kall

--
Dr. rer. nat. Jochen Kall

Funktionale Sicherheit

ITK Engineering GmbH
Im Speyerer Tal 6
76761 Rülzheim

Tel.: +49 7272 7703-546
Fax: +49 7272 7703-100

Mobil:+491734957776

mailto:jochen.kall@itk-engineering.de ( jochen.kall@itk-engineering.de )

______________________________________________________________

ITK Engineering GmbH | Im Speyerer Tal 6 | 76761 Rülzheim

Tel.: +49 7272 7703-0 | Fax: +49 7272 7703-100

mailto:info@itk-engineering.de ( info@itk-engineering.de ) | http://www.itk-engineering.de ( http://www.itk-engineering.de/ )

Vorsitzender des Aufsichtsrats/Chairman of the Supervisory Board:

Dr. Rudolf Maier

Geschäftsführung/Executive Board:

Michael Englert (Vorsitzender/Chairman), Bernd Gohlicke

Sitz der Gesellschaft/Registered Office: 76761 Rülzheim

Registergericht/Registered Court: Amtsgericht Landau, HRB 32046

USt.-ID-Nr./VAT-ID-No. DE 813165046