Re: Friday is for fun: fixing the error paths in selinux


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@...>
- 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@...> 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@... <devel@...> Im Auftrag von Lukas
Bulwahn
Gesendet: Freitag, 4. Dezember 2020 11:35
An: devel@...
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@... ( jochen.kall@... )

______________________________________________________________

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

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

mailto:info@... ( info@... ) | 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

Join devel@lists.elisa.tech to automatically receive all group messages.