From: Alexandre Ratchov (alexcaoua.org)
Date: Tue Jan 22 2008 - 13:08:20 CST

• Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Tue, Jan 22, 2008 at 12:10:50PM +0100, Remco wrote:
> I noticed that the uaudio_get() function in uaudio.c uses a 4 byte buffer for
> data transfer. If I understand the code correctly it also takes the length of
> the buffer as a parameter (int len).
>
> This way it seems possible to request data of an an arbitrary length to be put
> in a 4 byte buffer.
>
> Even though it seems this function doesn't get called with a length mismatch,
> I thought an extra check might be in order:
>
> Index: uaudio.c
> ===================================================================
> RCS file: /cvs/src/sys/dev/usb/uaudio.c,v
> retrieving revision 1.56
> diff -u -r1.56 uaudio.c
> --- uaudio.c 2 Dec 2007 14:59:31 -0000 1.56
> +++ uaudio.c 22 Jan 2008 11:07:58 -0000
> -2197,10 +2197,15
> int wIndex, int len)
> {
> usb_device_request_t req;
> - u_int8_t data[4];
> + #define UAUDIO_GET_BUF_SIZE 4
> + u_int8_t data[UAUDIO_GET_BUF_SIZE];
> usbd_status err;
> int val;
>
> + if ( (len < 0) || (len > UAUDIO_GET_BUF_SIZE) ) {
> + DPRINTF(("uaudio_get: bad length=%d\n", len));
> + return (-1);
> + }
> if (wValue == -1)
> return (0);

well, we can't (and afaik we don't want to) check parameters of
every function that takes a length argument; imo it's not useful to
do so for functions that aren't in an insecure (or uncertain) code
path. Functions that process data generated by ourselves don't need
to be checked, especially if the code that generated the data is
simple.

As you said, in this particular case, uaudio_get() is used only to
get 8-bit or 16-bit integers (used by the mixer), so 'len' is
either sizeof(uint8_t) or sizeof(uint16_t). Indeed, we always have
len = MIX_SIZE(type) and the MIX_SIZE() macro is defined as:

#define MIX_SIZE(n) ((n) == MIX_SIGNED_16 || (n) == MIX_UNSIGNED_16 ? 2 : 1)

-- Alexandre

• Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]