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

Add command to display Android binder driver information #1488

Draft
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

gsingh93
Copy link
Member

@gsingh93 gsingh93 commented Jan 2, 2023

Adds the binder command to display information related to the Binder driver on Android devices:
image

It works well, but the code is a mess and needs to be cleaned up before merging.

@Notselwyn
Copy link

Any updates here? Seems like a great feature to upstream, as I missed this significantly during kernel research.

@gsingh93
Copy link
Member Author

Thanks for the interest @Notselwyn. It's unlikely I'll get time to work on this in the near future as I wrote it for some security research that's been completed, so I don't have any bandwidth to return to it. I uploaded it here in an unfinished state because I thought at least anyone interested could still use it, which would be better than keeping it to myself.

I'd be hesitant to merge it as is, as there are a number of unimplemented things and likely broken edge cases. We'd get bug reports from users that we wouldn't be able to support. But hopefully someone else will be able to pick it up at some point.

@disconnect3d
Copy link
Member

disconnect3d commented Jun 9, 2024

@gsingh93

I'd be hesitant to merge it as is, as there are a number of unimplemented things and likely broken edge cases. We'd get bug reports from users that we wouldn't be able to support. But hopefully someone else will be able to pick it up at some point.

Can we maybe add a print at the end along the lines of "hey, its not finished, help us improving it" and merge it?

@fidgetingbits
Copy link
Contributor

@gsingh93

I'd be hesitant to merge it as is, as there are a number of unimplemented things and likely broken edge cases. We'd get bug reports from users that we wouldn't be able to support. But hopefully someone else will be able to pick it up at some point.

Can we maybe add a print at the end along the lines of "hey, its not finished, help us improving it" and merge it?

I wonder if it's worth having an experimental category for commands like this that indicates to users they should expect it to be broken or unfinished or subject to breaking changes, etc.

@Notselwyn
Copy link

I wonder if it's worth having an experimental category for commands like this that indicates to users they should expect it to be broken or unfinished or subject to breaking changes, etc.

I'm sorry for mixing in and I have no authority on this matter, but I would feel like this is the go-to. As a pwndbg user I wouldn't care enough to locally apply the patches from this PR: I'd be wayy happier with a little banner that says the command is experimental with a link to the PR, as I like something that just works without a lot of effort (even if it may break at some point)

Perhaps it would be good to do this approach for #1487 as well :-)

@gsingh93
Copy link
Member Author

Thanks for the feedback. I'll add both this command and the pcp commmand as experimental commands in the next week or two. Note that I'm not planning on testing them at all before merging them (even if there were bugs, I wouldn't have time to fix them), so just note that the last kernel versions I know they worked on were whatever was used for this exploit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

None yet

4 participants