sound: Implement SNDCTL_SETNAME

[christos]

Summary

Reference: http://manuals.opensound.com/developer/SNDCTL_SETNAME.html

Sponsored by: The FreeBSD Foundation
MFC after: 2 weeks

[markj]

sys/dev/sound/pcm/dsp.c

1877

This is dangerous. You're taking a 64-byte buffer supplied by userspace and assuming that it's a nul-terminated C string, but there's no check which verifies that (as far as I can see anyway).

Aside from that, we should probably also ensure that characters in the string are "sane", e.g., they all satisfy isprint().

[dev_submerge.ch]

Do you have a particular interest in this ioctl? I didn't find any software using it on github, just example code from the 4Front OSS docs. Maybe it's a good opportunity to add an OSS API test executable now? Otherwise I have to hack it into sosso or something to run it.

Also probably needs rebase, doesn't apply with git arc patch on main.

[imp]

sys/dev/sound/pcm/dsp.c

1877

May want to disallow double quotesas well. Though it might be ok for the description.

But we need sane UTF-8 strings rather than just isprint, no?

[markj]

sys/dev/sound/pcm/dsp.c

1877

Aren't all isprint() strings valid UTF-8 strings?

[christos]

Address Mark's comments. NUL-terminate the input string in general, but also
NUL-terminate it right before we encounter a non-isprint() character (or do we
want to return EINVAL in this case?)

[christos]

In D44570#1016366, @dev_submerge.ch wrote:

Do you have a particular interest in this ioctl?

I really just want to implement them for completeness. There is no reason not to implement them, plus they are easy to implement.

I didn't find any software using it on github, just example code from the 4Front OSS docs. Maybe it's a good opportunity to add an OSS API test executable now? Otherwise I have to hack it into sosso or something to run it.

I don't know if I will do it right now, but I am definitely planning on writing some kind of OSS API test suite/program in the near future.

Also probably needs rebase, doesn't apply with git arc patch on main.

I just applied it without problems. Do you still have this issue?

[christos]

sys/dev/sound/pcm/dsp.c

1881

I guess we can just break here as well, although it'll have the same effect.

[markj]

sys/dev/sound/pcm/dsp.c

1881

This doesn't fix the problem. How do you know that s points into the bounds of the buffer?

Regarding error handling, I would be strict and simply return EINVAL if

1. the buffer is not nul-terminated, or
2. the buffer contains unprintable characters.

[christos]

Address Mark's comment.

[markj]

sys/dev/sound/pcm/dsp.c

1881

This must be strnlen(). Otherwise a user-supplied buffer that doesn't contain a nul-terminator will cause strlen() to keep going past the end of the buffer. That can cause a kernel panic.

[christos]

Address Mark's comment. Sorry for this.

[dev_submerge.ch]

Hacked it into the sosso test executable to exercise this ioctl, I'm still not convinced of its utility. This lets every application of all users set the device description globally. I think it's useful on a system level (mixer or similar tool), but I'd be wary to use it in an application. For the purposes mentioned in OSSv4, something like virtual_oss can set the description of the pcm devices it creates in a different way.

sys/dev/sound/pcm/dsp.c

1881

Please note that strnlen() could return 0 or sizeof(oss_longname_t), in both cases we're reading out of bounds.
And the first character is not checked for isprint().
Why not something simpler like this (untested!):

for (len = 0; len < sizeof(oss_longname_t) - 1; len++) {
  if (!isprint(s[len]))
    break;
}
if (s[len] != '\0')
  ret = EINVAL;

Also consider to factor this out into a separate function, for readability and reuse.

[christos]

In D44570#1018022, @dev_submerge.ch wrote:

Hacked it into the sosso test executable to exercise this ioctl, I'm still not convinced of its utility. This lets every application of all users set the device description globally. I think it's useful on a system level (mixer or similar tool), but I'd be wary to use it in an application. For the purposes mentioned in OSSv4, something like virtual_oss can set the description of the pcm devices it creates in a different way.

As I said already, I also do not think these IOCTLs are actually important at least for now, but I wanted to implement them for completeness. However, I am not sure if it's a good allocation of resources, so maybe it's better to abandon these for now.