Implement 32 bit compatibility for the sound ioctls that need it, as per bug 216568.
Details
Details
A test case is attached to bug 216568.
Diff Detail
Diff Detail
- Repository
- rG FreeBSD src repository
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
| sys/dev/sound/pcm/dsp.c | ||
|---|---|---|
| 717 | I suggest we put all that in sys/sys/soundcard.h, and instead of defining additional structures, just add ifdefs around the fields you are replacing to 32-bit ints. It might also make the implementation in dsp_ioctl() cleaner. | |
Comment Actions
This patch makes a number of improvements:
- The 32 bit structs are moved to sys/sys/soundcard.h, similar to struct sndstioc_nv_arg32 in sys/sys/sndstat.h (although most COMPAT_FREEBSD32 code in the kernel doesn't add 32 bit structs to header files, so why are we?).
- _IOC_NEWTYPE is used instead of repeating the ioctl definitions with the new type.
- The dsp_ioctl() function stays unchanged, instead a wrapper function dsp_ioctl_compat() does data conversions before and/or after calling dsp_ioctl(), for the structs that need conversion. This also has the benefit of better performance, as all the conversion is done outside of the locking done by dsp_ioctl(). And it's much cleaner, only a single COMPAT_FREEBSD32 ifdef is used.
- I've removed snd_sync_parm32, because even snd_sync_parm is not really implemented.
- Lines wrapped to 80 characters.
Comment Actions
Instead of defining 32-bit structures and ioctls with different names, why don't you just have an ifndef COMPAT_FREEBSD32 in the structure itself? Something like:
typedef struct _snd_chan_param {
#ifndef COMPAT_FREEBSD32
u_long play_rate; /* sampling rate */
u_long rec_rate; /* sampling rate */
u_long play_format; /* everything describing the format */
u_long rec_format; /* everything describing the format */
#else
u_int32_t play_rate; /* sampling rate */
u_int32_t rec_rate; /* sampling rate */
u_int32_t play_format; /* everything describing the format */
u_int32_t rec_format; /* everything describing the format */
#endif
} snd_chan_param;This way you could avoid both the additional logic in dsp_ioctl(), as well as the additional struct and ioctl definitions. Or am I missing something and this is not possible?
Also I do not really like the 32-bit specific logic you are introducing here with dsp_ioctl_compat(), which then also calls dsp_ioctl() manually. If what I'm proposing can work, then you'd essentially have the same result and no additional logic inside sound(4).
| sys/sys/soundcard.h | ||
|---|---|---|
| 259 ↗ | (On Diff #162082) | I would omit the defined(_KERNEL) parts here and below. |
Comment Actions
If we did this:
typedef struct _snd_chan_param {
#ifndef COMPAT_FREEBSD32
u_long play_rate; /* sampling rate */
u_long rec_rate; /* sampling rate */
u_long play_format; /* everything describing the format */
u_long rec_format; /* everything describing the format */
#else
u_int32_t play_rate; /* sampling rate */
u_int32_t rec_rate; /* sampling rate */
u_int32_t play_format; /* everything describing the format */
u_int32_t rec_format; /* everything describing the format */
#endif
} snd_chan_param;then there would only be a 64 bit version of the struct, or only a 32 version of the struct, depending on COMPAT_FREEBSD32. We want both the 64 bit version for 64 bit callers, and the 32 bit version for 32 bit callers, to coexist, which requires different structs.
We could avoid different structs in C++ by using templates, but in C the best we could do is define the 2 versions of the same struct through an ugly macro:
#define SND_CHAN_PARAM_DEF(T, NAME) typedef struct _##NAME { \
T play_rate; /* sampling rate */ \
T rec_rate; /* sampling rate */ \
T play_format; /* everything describing the format */ \
T rec_format; /* everything describing the format */ \
} NAME;
#ifdef COMPAT_FREEBSD32
SND_CHAN_PARAM_DEF(u_int32_t, snd_chan_param32)
#endif
SND_CHAN_PARAM_DEF(u_long, snd_chan_param)My approach is used in many other places in the kernel, for example look at how in6_ndifreq32 is used in /usr/src/sys/netinet6/in6.c.
Comment Actions
Right, that makes sense. I forgot that we want to support both at the same time.
However I still insist on the dsp_ioctl() changes. I would make the 32 and 64 bit ioctls fallthrough, and either use the 32 or the 64 bit structs depending on the actual ioctl that was used.
Comment Actions
If this is what you meant, it does seems considerably longer and uglier, for example:
case SNDCTL_DSP_GETERROR:
#ifdef COMPAT_FREEBSD32
case SNDCTL_DSP_GETERROR32:
#endif
/*
* OSSv4 docs: "All errors and counters will automatically be
* cleared to zeroes after the call so each call will return only
* the errors that occurred after the previous invocation. ... The
* play_underruns and rec_overrun fields are the only useful fields
* returned by OSS 4.0."
*/
{
#ifdef COMPAT_FREEBSD32
if (cmd == SNDCTL_DSP_GETERROR32)
{
audio_errinfo32 *ei = (audio_errinfo32 *)arg;
bzero((void *)ei, sizeof(*ei));
if (wrch != NULL) {
CHN_LOCK(wrch);
ei->play_underruns = wrch->xruns;
wrch->xruns = 0;
CHN_UNLOCK(wrch);
}
if (rdch != NULL) {
CHN_LOCK(rdch);
ei->rec_overruns = rdch->xruns;
rdch->xruns = 0;
CHN_UNLOCK(rdch);
}
}
else
#endif
{
audio_errinfo *ei = (audio_errinfo *)arg;
bzero((void *)ei, sizeof(*ei));
if (wrch != NULL) {
CHN_LOCK(wrch);
ei->play_underruns = wrch->xruns;
wrch->xruns = 0;
CHN_UNLOCK(wrch);
}
if (rdch != NULL) {
CHN_LOCK(rdch);
ei->rec_overruns = rdch->xruns;
rdch->xruns = 0;
CHN_UNLOCK(rdch);
}
}
}
break;Comment Actions
This is just a quick draft, haven't tested that so take this with a grain of salt, but I guess something like this could work?
case SNDCTL_DSP_GETERROR:
#ifdef COMPAT_FREEBSD32
case SNDCTL_DSP_GETERROR32:
#endif
/*
* OSSv4 docs: "All errors and counters will automatically be
* cleared to zeroes after the call so each call will return only
* the errors that occurred after the previous invocation. ... The
* play_underruns and rec_overrun fields are the only useful fields
* returned by OSS 4.0."
*/
{
audio_errinfo *ei = (audio_errinfo *)arg;
bzero((void *)ei, sizeof(*ei));
if (wrch != NULL) {
CHN_LOCK(wrch);
ei->play_underruns = wrch->xruns;
wrch->xruns = 0;
CHN_UNLOCK(wrch);
}
if (rdch != NULL) {
CHN_LOCK(rdch);
ei->rec_overruns = rdch->xruns;
rdch->xruns = 0;
CHN_UNLOCK(rdch);
}
#ifdef COMPAT_FREEBSD32
if (cmd == SNDCTL_DSP_GETERROR32) {
audio_errinfo32 ei32;
ei32.play_underruns = ei->play_underruns;
ei32.rec_underruns = ei->rec_underruns;
bzero((void *)ei, sizeof(*ei));
*(audio_errinfo32 *)arg = ei32;
}
#endif
}
break;Just to be clear, I do not really have a huge objection to what you are proposing, but if we could make the code a bit more self-contained (as in, the compat shim is next to the regular ioctl code), I think that'd be nice. But if my proposal ends up making things even more complex then we can go on with the current one.
Comment Actions
This corrupts memory and is a possible security exploit: zero-filling a smaller 32 bit struct with sizeof(larger 64 bit struct) zeroes, will writes zeroes beyond the end of the struct.
If we are going to take that route, either we have to copy channels -> variables, then variables -> correct struct:
{
int play_underruns = 0;
int rec_overruns = 0;
if (wrch != NULL) {
CHN_LOCK(wrch);
play_underruns = wrch->xruns;
wrch->xruns = 0;
CHN_UNLOCK(wrch);
}
if (rdch != NULL) {
CHN_LOCK(rdch);
rec_overruns = rdch->xruns;
rdch->xruns = 0;
CHN_UNLOCK(rdch);
}
if (compat32) { /* which would be set earlier somewhere */
#ifdef COMPAT_FREEBSD32
audio_errinfo32 *ei = (audio_errinfo32 *)arg;
bzero((void *)ei, sizeof(*ei));
ei->play_underruns = play_underruns;
ei->rec_overruns = rec_overruns;
#endif
} else {
audio_errinfo *ei = (audio_errinfo *)arg;
bzero((void *)ei, sizeof(*ei));
ei->play_underruns = play_underruns;
ei->rec_overruns = rec_overruns;
}
}
break;or we have to do what I did in the first patch here, convert the 32 bit struct to a new 64 bit struct if necessary, run the 64 bit code on it, then convert back to 32 bit if necessary:
{
audio_errinfo *ei = (audio_errinfo *)arg;
#ifdef COMPAT_FREEBSD32
audio_errinfo errinfo;
audio_errinfo32 *ei32 = (audio_errinfo32 *)arg;
if (cmd == SNDCTL_DSP_GETERROR32) {
ei = &errinfo;
}
#endif
bzero((void *)ei, sizeof(*ei));
if (wrch != NULL) {
CHN_LOCK(wrch);
ei->play_underruns = wrch->xruns;
wrch->xruns = 0;
CHN_UNLOCK(wrch);
}
if (rdch != NULL) {
CHN_LOCK(rdch);
ei->rec_overruns = rdch->xruns;
rdch->xruns = 0;
CHN_UNLOCK(rdch);
}
#ifdef COMPAT_FREEBSD32
if (cmd == SNDCTL_DSP_GETERROR32) {
*ei32 = *(audio_errinfo32 *)ei;
ei32->play_errorparm = ei->play_errorparm;
ei32->rec_errorparm = ei->rec_errorparm;
}
#endif
}
break;Comment Actions
That's why I said take this with a grain of salt. I wrote it just to showcase what I meant.
or we have to do what I did in the first patch here, convert the 32 bit struct to a new 64 bit struct if necessary, run the 64 bit code on it, then convert back to 32 bit if necessary:
{ audio_errinfo *ei = (audio_errinfo *)arg; #ifdef COMPAT_FREEBSD32 audio_errinfo errinfo; audio_errinfo32 *ei32 = (audio_errinfo32 *)arg; if (cmd == SNDCTL_DSP_GETERROR32) { ei = &errinfo; } #endif bzero((void *)ei, sizeof(*ei)); if (wrch != NULL) { CHN_LOCK(wrch); ei->play_underruns = wrch->xruns; wrch->xruns = 0; CHN_UNLOCK(wrch); } if (rdch != NULL) { CHN_LOCK(rdch); ei->rec_overruns = rdch->xruns; rdch->xruns = 0; CHN_UNLOCK(rdch); } #ifdef COMPAT_FREEBSD32 if (cmd == SNDCTL_DSP_GETERROR32) { *ei32 = *(audio_errinfo32 *)ei; ei32->play_errorparm = ei->play_errorparm; ei32->rec_errorparm = ei->rec_errorparm; } #endif } break;
This looks good. If the rest of the ioctls do not end up getting too complicated with this approach, we could go ahead with this.
| sys/sys/soundcard.h | ||
|---|---|---|
| 259 ↗ | (On Diff #162082) | Fair enough. I was the one who proposed moving those from dsp.c to soundcard.h in order to have all definitions in one place, and not clutter dsp.c. |
Comment Actions
Thank you for the feedback. This new patch:
- Moves the 32 bit structs from sys/soundcard.h to dsp.c.
- Uses tabs for indentation.
- _IOC_NEWTYPE is used instead of repeating the ioctl definitions with the new type.
- Removes snd_sync_parm32 and AIOSYNC32, because snd_sync_parm and AIOSYNC are not really implemented.
- Only converts from 32 to 64 bit on AIOSFMT32, not on AIOGFMT32 where it was unnecessary.
- Converts from 32 to 64 bit on AIOGCAP32, unlike before, because all-zero fields have a meaning to the ioctl.
| sys/dev/sound/pcm/dsp.c | ||
|---|---|---|
| 691 | I suggest to use fixed-width types, not depending on the ABI. E.g. use uint16_t instead of u_short, int32_t instead of int etc. | |
| 1005 | I do not quite understand this. p32 has the limited scope of the enclosing {} block, so it is unreachable right after the case is done executing. What is the point of calculating *p32 then? | |
| 1743 | Same question there. | |
| sys/dev/sound/pcm/dsp.c | ||
|---|---|---|
| 1005 | Previously: snd_capabilities32 *p32 = (snd_capabilities32 *)arg;
snd_capabilities capabilities;
if (cmd == AIOGCAP32) {
p = &capabilities;so for AIOGCAP32, p points to temporary stack memory, and p32 (eventually) writes back to userspace. | |
Comment Actions
This patch also uses C standard names for struct field types, and properly initializes and copies fields to audio_errinfo32.
Comment Actions
I'm not sure if this compiles (see inline comment), but in any case, I will test the patch tomorrow. If you do not have a commit bit, I will also take care of it, and make sure it's MFC'd.
| sys/dev/sound/pcm/dsp.c | ||
|---|---|---|
| 1767–1768 | Those are mixed up. | |