linux-kernel - Re: question about drivers/media/usb/gspca/kinect.c

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:	Mon, 30 Dec 2013 16:56:25 +0100
From:	Antonio Ospite <ospite@...denti.unina.it>
To:	Julia Lawall <julia.lawall@...6.fr>
Cc:	hdegoede@...hat.com, m.chehab@...sung.com,
	linux-media@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: question about drivers/media/usb/gspca/kinect.c

On Wed, 25 Dec 2013 20:00:34 +0100 (CET)
Julia Lawall <julia.lawall@...6.fr> wrote:

> The following code, in the function send_cmd, looks too concise:
> 
>         do {
>                 actual_len = kinect_read(udev, ibuf, 0x200);
>         } while (actual_len == 0);
>         PDEBUG(D_USBO, "Control reply: %d", res);
>         if (actual_len < sizeof(*rhdr)) {
>                 pr_err("send_cmd: Input control transfer failed (%d)\n", res);
>                 return res;
>         }
> 
> It seems that actual_len might be less than sizeof(*rhdr) either because 
> an error code is returned by the call to kinect_read or because a shorter 
> length is returned than the desired one.  In the error code case, I would 
> guess that one would want to return the error code, but I don't know what 
> on would want to return in the other case.  In any case, res is not 
> defined by this code, so what is returned is whatever the result of the 
> previous call to kinect_write happened to be.
> 
> How should the code be changed?
>

Thanks Julia,

some other drivers return -EIO when the actual transfer length does not
match the requested one[1], and from Documentation/usb/error-codes.txt
[2] it looks like -EREMOTEIO is also used to represent partial
transfers in some cases. So I'd say either one of the two is OK.

The interested code is almost the same used in libfreenect[3], so I'd
stay with a minimal change here:

diff --git a/drivers/media/usb/gspca/kinect.c b/drivers/media/usb/gspca/kinect.c
index 3773a8a..48084736 100644
--- a/drivers/media/usb/gspca/kinect.c
+++ b/drivers/media/usb/gspca/kinect.c
@@ -158,7 +158,7 @@ static int send_cmd(struct gspca_dev *gspca_dev, uint16_t cmd, void *cmdbuf,
        PDEBUG(D_USBO, "Control reply: %d", res);
        if (actual_len < sizeof(*rhdr)) {
                pr_err("send_cmd: Input control transfer failed (%d)\n", res);
-               return res;
+               return actual_len < 0 ? actual_len : -EREMOTEIO;
        }
        actual_len -= sizeof(*rhdr);

Proper patches on their way, to libfreenect too.

Thanks again,
   Antonio

[1]
http://lxr.linux.no/#linux+v3.12.6/drivers/media/usb/dvb-usb-v2/dvb_usb_urb.c#L37
[2]
http://lxr.linux.no/#linux+v3.12.6/Documentation/usb/error-codes.txt#L134
[3]
https://github.com/OpenKinect/libfreenect/blob/master/src/flags.c#L87

-- 
Antonio Ospite
http://ao2.it

A: Because it messes up the order in which people normally read text.
   See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/