Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[r-mr1] radio: Allow finding qti uce service #597

Merged
merged 3 commits into from Mar 28, 2021

Conversation

ix5
Copy link
Contributor

@ix5 ix5 commented Mar 18, 2021

radio: Allow finding qti.uceservice

qcrild as part of the radio domain needs to find com.qualcomm.qti.uceservice::IUceService

Move public definitions back to vendor

There was no need to keep the definitions shared between system and vendor when the domains using them were contained in our vendor sepolicy only.

Revert shell dmesg

Original commit was typo'd, it meant to allow shell, not system_server to read syslog.
However, that'd be a neverallow:
system/sepolicy/domain.te:519:

neverallow appdomain kernel:system { syslog_read syslog_mod syslog_console };

system/sepolicy/private/shell.te:22:

.# XXX Transition into its own domain?
app_domain(shell)

Copy link
Contributor

@MarijnS95 MarijnS95 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a hunch this should be:

hal_attribute_hwservice(hal_telephony, vnd_qti_uce_hwservice)

In hal_telephony.te.

@ix5 ix5 force-pushed the radio-qti-uce branch 2 times, most recently from e60730d to 8472ab9 Compare March 20, 2021 12:13
@ix5
Copy link
Contributor Author

ix5 commented Mar 20, 2021

@MarijnS95 might taking another look?

@ix5 ix5 marked this pull request as ready for review March 20, 2021 12:17
@MarijnS95
Copy link
Contributor

@ix5 That's pretty crazy, shouldn't ims be a server domain to be allowed to add (host) IUCEService? Or was that wrong judgement by 346be22?

If it is, please:

Fixes: 346be22 ("ims: Label new services and grant add_hwservice access.")

But I don't see any other domain with the rights to find/host this service... Perhaps because I have never used this and anyone doing so (PE) is still in permissive.

  • Who hosts this service?
  • Do we at some point need a new hwservice "attribute" to shove ims-specific services under?

Copy link
Contributor

@MarijnS95 MarijnS95 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

akatsuki:/ # lshal | grep -i uce
X     Y android.hidl.base@1.0::IBase/com.qualcomm.qti.uceservice                                  0/1        770    518
DM    Y com.qualcomm.qti.uceservice@2.0::IUceService/com.qualcomm.qti.uceservice                  0/1        770    518
DM    Y com.qualcomm.qti.uceservice@2.1::IUceService/com.qualcomm.qti.uceservice                  0/1        770    518
akatsuki:/ # ps -A 770
USER           PID  PPID     VSZ    RSS WCHAN            ADDR S NAME
system         770     1   59588   4536 binder_io+          0 S imsrcsd

So imsrcsd hosts it, which is ims_exec, so ims. Should be a server domain, did you run-test this on-device?

vendor/ims.te Outdated Show resolved Hide resolved
@MarijnS95
Copy link
Contributor

We should get @Paulbouchara to test this in parallel with us on PE since they use IMS and go enforcing soon™, in particular the remaining add_hwservices should probably be moved to hal_attribute_hwservice while falling under the hal_server_domain(ims, hal_telephony) catch-all.

Iirc hwbinder_use at the bottom of ims.te is granted by one of these macros as well?

@Paulbouchara
Copy link

We should get @Paulbouchara to test this in parallel with us on PE since they use IMS and go enforcing soon™, in particular the remaining add_hwservices should probably be moved to hal_attribute_hwservice while falling under the hal_server_domain(ims, hal_telephony) catch-all.

Iirc hwbinder_use at the bottom of ims.te is granted by one of these macros as well?

The next PE release will be on enforcing, as every releases that will follow, so sure, I'll test this asap when I'll be back and report with logs

This reverts commit bd5eaf8.

Reason for revert:
Original commit was typo'd, it meant to allow `shell`, not
`system_server` to read syslog.
However, that'd be a neverallow:
`system/sepolicy/domain.te:519`:
```
neverallow appdomain kernel:system { syslog_read syslog_mod syslog_console };
```
`system/sepolicy/private/shell.te:22`:
```
.# XXX Transition into its own domain?
app_domain(shell)
```
@MarijnS95
Copy link
Contributor

MarijnS95 commented Mar 20, 2021

@ix5 One more thing: the denial shown in the logs tries to find (and perhaps add later) this service for the radio domain, but this PR allows hosting (adding) it from the ims domain...

ims adds/hosts it, radio (why it is added to hal_telephony here) finds/uses it?

@ix5
Copy link
Contributor Author

ix5 commented Mar 20, 2021

@MarijnS95 ims was already hosting the service via the add_hwservice() macro.

That macro, however, forbids any other domains (even parent ones) from adding the service. So ims was not hosting this service in a server capacity, but just on its own.

The hal_attribute_hwservice() macro adds add_hwservice($1_server, $2), which would clash with add_hwservice(ims, vnd_qti_uce_hwservice).

The only way for a coredomain such as radio to access the IUceService is via hal_ server/client attribute empowerment.
As such, we need to keep the status quo (ims still hosts the service), just change the language.

-> Not true, we can still allow radio find find the service, just not add it (which is not needed anyway).

@MarijnS95
Copy link
Contributor

MarijnS95 commented Mar 21, 2021

Final verdict: radio only needs to find the service (and have binder communication with the domain hosting it, ims) so we're better off not adding it to hal_telephony since then:

  • radio is a server domain, and would incorrectly be allowed to host it;
  • ims would become a server domain, and is incorrectly allowed to host all other services in the list.

As posted in master...MarijnS95:ims-attribute-group we should check whether more hwservices in the hal_telephony domain are actually not hosted by radio at all, and are better separated so that radio can only read them.

(Note that qcrild runs under the radio domain)

ix5 added 2 commits March 22, 2021 23:02
There was no need to keep the definitions shared between
system and vendor when the domains using them were contained
in our vendor sepolicy only.
Denial:
avc: denied  { find } for \
    interface=com.qualcomm.qti.uceservice::IUceService \
    sid=u:r:radio:s0 scontext=u:r:radio:s0 \
    tcontext=u:object_r:vnd_qti_uce_hwservice:s0 \
    tclass=hwservice_manager
@ix5
Copy link
Contributor Author

ix5 commented Mar 22, 2021

For posterity, this was the original description of the "solution" using HAL attributes we discarded:


This accomplishes the following:

  • Wrestle control over vnd_qti_uce_hwservice from ims domain
    (add_hwservice() macros includes a neverallow for other domains to add the service in any way)
  • All clients of hal_telephony are now allowed to access com.qualcomm.qti.uceservice::IUceService
    This includes radio, which is a client domain of hal_telephony.

Since ims is no longer allowed access to vnd_qti_uce_hwservice, it needs to be converted into a server of hal_telephony via the hal_server_domain() macro.
N.b.: imsrcsd hosts the IUceService, so it really needs to be a server and not only a client.

Denial:

avc: denied  { find } for interface=com.qualcomm.qti.uceservice::IUceService \
    sid=u:object_r:radio:s0 scontext=u:object_r:radio:s0 \
    tcontext=u:object_r:vnd_qti_uce_hwservice:s0 tclass=hwservice_manager

Plus a small fixup

@jerpelea jerpelea merged commit 8884b0d into sonyxperiadev:r-mr1 Mar 28, 2021
@ix5 ix5 deleted the radio-qti-uce branch March 29, 2021 07:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants