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;-)

[Flamefire]

I see, there is an array of size MAX_APP_NAME_SIZE which is 64, so that's fine

Making this discussion academical

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.

Added a couple minor change suggestions which IMO make sense. E.g. getting the rb pointer only when it will be retained and making the logic easier to follow. Besides that basically just matching style and directly casting to the target type instead of void* which makes the below lines easier to reason about.

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 ;) )

[Flamefire]

Suggested change

	rb = (void *)__qseecom_uvirt_to_kvirt(data,
	(uintptr_t)req->resp_buf);
	if (sb != NULL && req->cmd_req_len >= sizeof(uint32_t) * 2)
	if (sb[0] != 0x07 || sb[1] != 0x04)
	sb = NULL;
	if (sb == NULL)
	rb = NULL;
	if (sb != NULL && req->cmd_req_len >= sizeof(uint32_t) * 2) {
	if (sb[0] == 0x07 && sb[1] == 0x04)
	rb = (uint32_t *)__qseecom_uvirt_to_kvirt(data, (uintptr_t)req->resp_buf);
	else
	sb = NULL;
	}

[j4nn]

In my opinion, this suggestion would not make any significant difference with code efficiency as the __qseecom_uvirt_to_kvirt() is simple arithmetic pointer conversion, not involving any memory mapping or whatever, so it's cost even if done unnecessary is nearing zero. I am not sure if this way the code would be more readable - to me it seems not really.
Concerning the cast to uint32_t * instead of void * - I guess that is a matter of taste.
I just followed the pattern already existing in the qseecom source code - you can see there casting to void * even though it is assigned to something else the same way I did it here - in fact, I copied it from the existing code.
So I am not sure what is better - to keep the usage uniform or try to invent something new.
In any case, there is no difference in the end.

[Flamefire]

__qseecom_uvirt_to_kvirt() is simple arithmetic pointer conversion,

Ok, then yes, doesn't affect efficiency.

I am not sure if this way the code would be more readable

It also adds the braces for the multi-line if code which is IMO required according to the style guide. But yeah, doesn't matter much

Concerning the cast to uint32_t * instead of void * - I guess that is a matter of taste.

Yeah, this is mostly about the adjacent usage of sizeof(uint32_t) and the access so one doesn't have to scroll up to the definition to verify this. Again, just minor

[j4nn]

A nested 'if' is not a problem without braces as long as there is no 'else' there. No 'else' in the original code, no braces. You have 'else' in your suggestion, so you have braces too.

[Flamefire]

Suggested change

	if (rb[0] == 0) {
	if (strncmp((uint8_t *)rb + 0x31,
	"HWC_Yoshino_Com_", 16) == 0)
	{
	((uint8_t *)rb)[0x30] = 1;
	// 0=not_allowed, 1=locked, 2=unlocked,
	// 3=allowed_when_sl_is_unlocked,
	// 4=allowed_since_sl_is_unlocked,
	// 5=unsupported_bl_status->generic error
	// (no info in security test screen "none")
	}
	}
	if (rb[0] == 0 && strncmp((uint8_t *)rb + 0x31, "HWC_Yoshino_Com_", 16) == 0) {
	((uint8_t *)rb)[0x30] = 1;
	// 0=not_allowed, 1=locked, 2=unlocked,
	// 3=allowed_when_sl_is_unlocked,
	// 4=allowed_since_sl_is_unlocked,
	// 5=unsupported_bl_status->generic error
	// (no info in security test screen "none")
	}

[j4nn]

This seems to be just formatting change - not really acceptable in my opinion.
I believe it is better to keep code lines shorter, less than 80 chars - is not that even required in kernel coding style to have the lines with less than 80 columns?

[Flamefire]

I mainly combined the nested if to an AND to reduce the nesting level which is a readability improvement and makes the check(s) explicit.
No idea about the 80 chars TBH, especially as TABs are used I find that hard to reason about...

[j4nn]

Yes, seen the combined conditions with the 'and'. Still in the end the difference is only at formatting level.
Which is wrong in your case - it is very simple: tabs are always considered 8 chars wide in kernel code and there is 80 columns limit per line.
See the kernel coding style, for example here:
https://www.kernel.org/doc/html/v4.10/process/coding-style.html

[Flamefire]

Suggested change

	sb = (void *)__qseecom_uvirt_to_kvirt(data,
	sb = (uint32_t *)__qseecom_uvirt_to_kvirt(data,

[j4nn]

As mentioned above - check in the upstream code, how __qseecom_uvirt_to_kvirt() is used. Using (void *) just keeps that style.

[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