hide sony bootloader unlock status in QSEE api call

[j4nn]

This patch forces bootloader unlock status returned from trust zone in sony proprietary api to be always as "locked". It does not fix kernel command line args that are set by verified bootloader with unlock related options and thus does not interfere with other android default verified boot policy. Instead it only fixes handling in original sony drm blobs that can be beneficial particularly if sony device (drm) key has been restored after unlock.

Without this patch, following log can be observed: libdevice_security_static: get_rooting_status.cpp:80 rooting_status 2 With this patch, following is logged instead: libdevice_security_static: get_rooting_status.cpp:80 rooting_status 1

This patch has been tested in today's build of los 17.1 in both TA partition states, i.e. lost and restored device key.

Please note also that the same api returns decrypted device key at offset 0x20 (16 bytes) if it has been restored in 66667 TA unit. If the device key has been lost and not restored, 16 zero bytes are returned at offset 0x20 instead. That means this way userspace proprietary libs may actually use the device key without directly reading it from the TA unit, if the still locked flag check is passed (just a theory without reverse engineering proof).

Change-Id: I4cea5b666377d71fb63d985839d095aa4240fb44

[j4nn]

Applies to (and tested with) derfelot:lineage-17.1_update 8ee9de4 commit (Merge Linux 4.4.235 kernel).

[derfelot]

since this doesn't have an effect on devices without restored keys, I'm fine with it. any objections @cryptomilk ?

[derfelot]

Oh @j4nn could you please change commit title to the like "qseecom: [current title]?

thanks

[j4nn]

Commit title changed as suggested, forced pushed, please pull the ca508e2 commit. Thanks.

[Flamefire]

I had this code in my build of this ROM a while ago and removed it again. I don't see what exactly it changes, i.e. no benefits in real use cases. I also see missing range checks which make me feel uncomfortable given that this is kernel code.

To be clear: I can confirm that the log output is rooting_status 1 (obviously as this is forced in there).

But integrity checks, bootloader lock state (during power on) etc are still failing and e.g. Magisk Hide is still required.

Can you provide an example how this actually benefits anything? Like something that works now which did not work before.

[linckandrea]

I had this code in my build of this ROM a while ago and removed it again. I don't see what exactly it changes, i.e. no benefits in real use cases. I also see missing range checks which make me feel uncomfortable given that this is kernel code.

To be clear: I can confirm that the log output is rooting_status 1 (obviously as this is forced in there).

But integrity checks, bootloader lock state (during power on) etc are still failing and e.g. Magisk Hide is still required.

Can you provide an example how this actually benefits anything? Like something that works now which did not work before.

"I don't see what exactly it changes"

"This patch forces bootloader unlock status returned from trust zone in sony proprietary api to be always as "locked"."

"It does not fix kernel command line args that are set by verified bootloader with unlock related options and thus does not interfere with other android default verified boot policy."

[j4nn]

@Flamefire could you please be more specific about the missing range checks? As far as I can see the qsee command structure is normally validated within __validate_send_cmd_inputs(), so that ensures request and response buffers are within the "shared buffer" including req and resp lengths. Basically we are peeking within that shared buffer and changing a byte only in case of a specific request together with magic string in the response.

Concerning benefits - you can see examples in the stock rom. With this patch you can get video enhancements and wifi display working, without the patch both do not work even when having TA-locked restored. It may be less useful with lineage os unless someone installs specific sony apps, but on the other hand, there are lot of sony proprietary blobs included within lineage os build, so it may actually have some influence as it even can be seen from the log, when the locked state is checked.

So why not to have it if someone has the drm key restored? Without this, the restored key is provably not used in some sony apps, disabling drm features not caring about presence of the key.

Also as mentioned in the commit log, the qsee api which returns the bl lock status returns also decrypted device key. Possibly sony proprietary/drm related blobs can check the lock status and use the decrypted device key only in case the status is still locked.

The decrypted device key returned at offset 0x20 is calculated from the last 16 bytes of hwconfig ta unit (0x7d3) xored by the 16 bytes from 66667 ta unit (the device key part which get's erased with bl unlock).

This way proprietary blobs can work with the decrypted device key without trying to read it directly from TA, making reverse engineering / finding where the key is used possibly more difficult than just checking which binary links to libmiscta.so.

Moreover as qseecom calls seem to be statically linked in many blobs since pie, it is also difficult to spot which binary calls that too. Because of this, this patch is the most easier way to "fix" the bl lock status in all sony blobs, without patching them and even without a need to search what would need to be patched.

[Flamefire]

could you please be more specific about the missing range checks?

I mean the code like if (strncmp((uint8_t *)rb + 0x31, "HWC_Yoshino_Com_", 16) == 0). How can you be sure there are 0x31+ bytes and there is a NULL terminated string or 16 more bytes at this point? AFAIK the checks done are basically a "hack" to detect the actual buffer to patch and don't wait for a specific packet with a known structure and size. Hence an OOB read could happen possibly leading to a bootloop. So this "peeking" without checking the size of the buffer could(!) be dangerous. So is this sure, that the size is enough? Why/How? Otherwise I'd suggest adding range checks.

so it may actually have some influence

That's what I meant: I'd like to see some influence. You mentioned "video enhancements and wifi display working". So seeing this with the ROM would be a good point.

To me "Possibly sony proprietary/drm related blobs can check the lock status" is not enough to warrant changing the kernel, which is a risk. A small one but still and it possibly incurs possible costs in resolving merge conflicts when the kernel is updated. I just want to make sure there is an actual (as opposed to possible) benefit in doing so.

Of course this is my opinion and nothing personal but a purely technical question, I hope you don't mind.

[j4nn]

I mean the code like if (strncmp((uint8_t *)rb + 0x31, "HWC_Yoshino_Com_", 16) == 0). How can you be sure there are 0x31+ bytes and there is a NULL terminated string or 16 more bytes at this point?

I agree it is a hack, but that is what you do when a vendor tries to limit usage of your own device. I will add the checks, that is no problem. They are most likely not needed, because qseecom api requires use of ION memory allocator for req & resp buffers and therefore the "shared buffer" in the qseecom driver is kernel page aligned including it's size as can be seen in __ion_alloc(): len = PAGE_ALIGN(len);

That's what I meant: I'd like to see some influence. You mentioned "video enhancements and wifi display working". So seeing this with the ROM would be a good point.

Fee free to make sony stock gallery with video playback working in los - I am not interested to do that. But I am very sure that you would see your proof in that case, as it happens with stock rom.

[j4nn]

Added the checks as suggested by @Flamefire. Retested to confirm it still works. Please see the new forced ee107e2 commit. Thanks.

[j4nn]

That follows the convention already used in the qseecom driver - see qseecom_unload_app(). That means if it was wrong, it would need to be patched there too.

Besides, if you wanted to try to crash the kernel from qseecom driver by oob read (which would not happen as there would not be a not mapped page boundary), you would need to have root already (or atleast tee_device selinux context), so there would be many much easier ways to crash the system, making this discussion academical;-)

[j4nn]

Sure, but as shown by history it usually is not one bug that causes the vulnerability but a chain of minor issues. That's why I wanted to be careful here.

Well the only write operation in the code is the changing of bl unlock state, after making very sure that it is exactly the response we want to modify. No way this could be escalated to anything. The mentioned theoretical possibility for kernel crash on out of boundary read is not existing in practice as the previous input args validation makes sure that all pointers are within the qseecom shared buffer, which is size aligned to page size in ion allocator. I have added the checks as suggested anyway, even though they are not needed in my opinion.

I hope you don't feel nagged by that, just want to make sure, everything is in the best possible shape, given that it is a kernel change :) Hence only a comment, not a condition for acceptance (not that I can enforce that in any way ;) )

I am ok with any suggestions, but I do not feel like those in the comment above are really worth of another commit and re-testing.

[Flamefire]

I am ok with any suggestions, but I do not feel like those in the comment above are really worth of another commit and re-testing.

Sure. That's why it is a suggestion and it may still help the maintainer(s) to reason about the change(s). I don't mind keeping it the way it is now.

[j4nn]

Thank you. It is always good if somebody reviews the code.

[Leo88gav]

how model sony? be was to xa2 sony pioneer?

[Flamefire]

@derfelot Are you willing to include this? IMO it's a useful addition