Re: [CFT][PATCH] signal/usb: Replace kill_pid_info_as_cred with kill_pid_usb_asyncio

[Eric W. Biederman]

 Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> writes:

> On Thu, 7 Feb 2019, Eric W. Biederman wrote:
>
>> The usb support for asyncio encoded one of it's values in the wrong
>> field.  It should have used si_value but instead used si_addr which is
>> not present in the _rt union member of struct siginfo.
>> 
>> The result is a POSIX and glibc incompatible encoding of fields in
>> struct siginfo with si_code of SI_ASYNCIO.  This makes it impossible
>> to look at a struct siginfo with si_code set to SI_ASYNCIO and without
>> context properly decode it.  Which unfortunately means that
>> copy_siginfo_to_user32 can't handle the compat issues this unfortunate
>> choice in encodings brings up.
>> 
>> Therefore replace kill_pid_info_as_cred with kill_pid_usb_asyncio a
>> dedicated function for this one specific case.  There are no other
>> users of kill_pid_info_as_cred so this specialization should have no
>> impact on the amont of code in the kernel.  Have kill_pid_usb_asyncio
>> take instead of a siginfo_t which is difficult error prone 3
>> arguments, a signal number, an errno value, and an address enconded as
>> a sigval_t.  The encoding as a sigval_t allows the caller to deal with
>> the compat issues before calling kill_pid_info_as_cred.
>> 
>> Add BUILD_BUG_ONs in kernel/signal.c to ensure that we can now place
>> the pointer value at the in si_pid (instead of si_addr) and get
>> the same binary result when the structure is copied to user space
>> and when the structure is copied field by field.
>> 
>> The usb code is updated to track if the values it passes into
>> kill_pid_usb_asyncio were passed to it from a native userspace
>> or from at compat user space.  To allow a proper conversion
>> of the addresses.
>> 
>> Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
>> Cc: linux-usb@xxxxxxxxxxxxxxx
>> Cc: Alan Stern <stern@xxxxxxxxxxxxxxxxxxx>
>> Cc: Oliver Neukum <oneukum@xxxxxxxx>
>> Fixes: v2.3.39
>> Cc: stable@xxxxxxxxxxxxxxx
>> Signed-off-by: "Eric W. Biederman" <ebiederm@xxxxxxxxxxxx>
>> ---
>> 
>> Can I get someone to test this code?  I just discovered that the
>> usb code is filling in struct siginfo wrong and copy_siginfo_to_user32
>> can't possibly get this correct without some help.
>> 
>> I think I have coded up a working fix.  But I don't have a setup
>> where I can test this.
>
> Eric:
>
> You should be able to test this patch by running the attached 
> program.  It takes one argument, the pathname to a USB device file.  
> For example, on my system:
>
> # ./usbsig /dev/bus/usb/001/001
> Got signal 10, signo 10 errno 0 code -4
>
> I don't know exactly what you're looking for, but it should be pretty 
> easy to modify the test program however you want.
>
> If you need to test the compatibility mode specifically, I can do that
> here -- I'm running a 32-bit userspace on a 64-bit kernel.  But you'll
> have to tell me exactly what test code to run.

Wow I got a little distracted but now I am back to this.

Using your test program I was able to test the basics of this.

I found one bug in my patch where I was missing a memset.  So I have
corrected that, and reorganized the patch a little bit.

I have not figured out how to trigger a usb disconnect so I have not
tested that.

The big thing I have not been able to test is running a 64bit big-endian
kernel with a 32bit user space.  My modified version of your test
program should report "Bad" without my patch, and should report "Good"
with it.

Is there any chance you can test that configuration?  I could not figure
out how to get a 64bit big-endian system running in qemu, and I don't
have the necessary hardware so I was not able to test that at all.  As
that is the actual bug I am still hoping someone can test it.

Thank you,
Eric Biederman
---
/* usbsig.c -- test USB async signal delivery */

#include <stdio.h>
#include <fcntl.h>
#include <signal.h>
#include <string.h>
#include <sys/ioctl.h>
#include <unistd.h>
#include <linux/usb/ch9.h>
#include <linux/usbdevice_fs.h>

static struct usbdevfs_urb urb;
static struct usbdevfs_disconnectsignal ds;

void urb_handler(int sig, siginfo_t *info , void *ucontext)
{
	printf("Got signal %d, signo %d errno %d code %d addr: %p urb: %p\n",
	       sig, info->si_signo, info->si_errno, info->si_code,
	       info->si_addr, &urb);

	printf("%s\n", (info->si_addr == &urb) ? "Good" : "Bad");
}

void ds_handler(int sig, siginfo_t *info , void *ucontext)
{
	printf("Got signal %d, signo %d errno %d code %d addr: %p ds: %p\n",
	       sig, info->si_signo, info->si_errno, info->si_code,
	       info->si_addr, &ds);

	printf("%s\n", (info->si_addr == &ds) ? "Good" : "Bad");
}

int main(int argc, char **argv)
{
	char *devfilename;
	int fd;
	int rc;
	struct sigaction act;
	struct usb_ctrlrequest *req;
	void *ptr;
	char buf[80];

	if (argc != 2) {
		fprintf(stderr, "Usage: usbsig device-file-name\n");
		return 1;
	}

	devfilename = argv[1];
	fd = open(devfilename, O_RDWR);
	if (fd == -1) {
		perror("Error opening device file");
		return 1;
	}

	act.sa_sigaction = urb_handler;
	sigemptyset(&act.sa_mask);
	act.sa_flags = SA_SIGINFO;

	rc = sigaction(SIGUSR1, &act, NULL);
	if (rc == -1) {
		perror("Error in sigaction");
		return 1;
	}

	act.sa_sigaction = ds_handler;
	sigemptyset(&act.sa_mask);
	act.sa_flags = SA_SIGINFO;

	rc = sigaction(SIGUSR2, &act, NULL);
	if (rc == -1) {
		perror("Error in sigaction");
		return 1;
	}

	memset(&urb, 0, sizeof(urb));
	urb.type = USBDEVFS_URB_TYPE_CONTROL;
	urb.endpoint = USB_DIR_IN | 0;
	urb.buffer = buf;
	urb.buffer_length = sizeof(buf);
	urb.signr = SIGUSR1;

	req = (struct usb_ctrlrequest *) buf;
	req->bRequestType = USB_DIR_IN | USB_TYPE_STANDARD | USB_RECIP_DEVICE;
	req->bRequest = USB_REQ_GET_DESCRIPTOR;
	req->wValue = USB_DT_DEVICE << 8;
	req->wIndex = 0;
	req->wLength = sizeof(buf) - sizeof(*req);

	rc = ioctl(fd, USBDEVFS_SUBMITURB, &urb);
	if (rc == -1) {
		perror("Error in SUBMITURB ioctl");
		return 1;
	}

	rc = ioctl(fd, USBDEVFS_REAPURB, &ptr);
	if (rc == -1) {
		perror("Error in REAPURB ioctl");
		return 1;
	}

	memset(&ds, 0, sizeof(ds));
	ds.signr = SIGUSR2;
	ds.context = &ds;
	rc = ioctl(fd, USBDEVFS_DISCSIGNAL, &ds);
	if (rc == -1) {
		perror("Error in DISCSIGNAL ioctl");
		return 1;
	}

	close(fd);
	return 0;
}