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

[Alan Stern]

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.

Alan Stern
/* 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>

void handler(int sig, siginfo_t *info , void *ucontext)
{
	ucontext_t *con = (ucontext_t *) ucontext;

	printf("Got signal %d, signo %d errno %d code %d\n",
			sig, info->si_signo, info->si_errno, info->si_code);
}

int main(int argc, char **argv)
{
	char *devfilename;
	int fd;
	int rc;
	struct sigaction act;
	struct usbdevfs_urb urb;
	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 = handler;
	sigemptyset(&act.sa_mask);
	act.sa_flags = SA_SIGINFO;

	rc = sigaction(SIGUSR1, &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;
	}

	close(fd);
	return 0;
}