ioctl DVD_READ_STRUCT fails for DVD_STRUCT_MANUFACT, causing some games to not detect their dvd?

Bug #777528 reported by Dan Kegel
12
This bug affects 1 person
Affects Status Importance Assigned to Milestone
Linux
Fix Released
Medium
linux (Ubuntu)
Fix Released
Undecided
Unassigned

Bug Description

In the test program

#include <fcntl.h>
#include <stdio.h>
#include <string.h>
#include <sys/ioctl.h>
#include <linux/cdrom.h>
main(int argc, char *argv[])
{
    dvd_struct buf;
    int fd;
    fd = open("/dev/sr0", O_RDONLY);
    if (fd < 0) {
        perror("open");
        return -1;
    }
    memset(&buf, 0, sizeof(buf));
    buf.type = DVD_STRUCT_MANUFACT;
    if (ioctl(fd, DVD_READ_STRUCT, &buf) < 0) {
        perror("DVD_READ_STRUCT");
        return -2;
    }
    printf("Got len %d, value '%*s'\n", buf.manufact.len,
           buf.manufact.len, buf.manufact.value);
    return 0;
}

the ioctl always fails with EIO. The test code seems too simple to be wrong,
so perhaps something is fishy in dvd_read_manufact() in the kernel,
http://lxr.linux.no/linux+v2.6.38/drivers/cdrom/cdrom.c#L1908

I care about this because this affects some games which are just trying
to verify that their DVD is inserted.

Rumor has it that the change

- if (s->manufact.len < 0 || s->manufact.len > 2048) {
+ if (s->manufact.len < 0 || s->manufact.len > 2052) {
   cdinfo(CD_WARNING, "Received invalid manufacture info length"
        " (%d)\n", s->manufact.len);
   ret = -EIO;
  } else {
- memcpy(s->manufact.value, &buf[4], s->manufact.len);
+ memcpy(s->manufact.value, &buf[4],
+ s->manufact.len > 2052 ? 2048 : s->manufact.len);

makes the ioctl function as expected, and lets the games detect
their dvd.

Revision history for this message
Scott Ritchie (scottritchie) wrote :

Do you have an upstream version of this bug?

Revision history for this message
Dan Kegel (dank) wrote :

No, I was hoping to do that once we had buy-in from ubuntu.

Revision history for this message
Dan Kegel (dank) wrote :

Or I suppose I could ask the guy who wrote the code,

^1da177e (Linus Torvalds 2005-04-16 15:20:36 -0700 1932) if (s->manufact.len < 0 || s->manufact.len > 2048) {

:-)

Revision history for this message
Igbaäl (cerebro-alexiel) wrote :

Have you filed a bug upstream ?
I'm looking forward to seeing this bug fixed as it would make lots of people able to run their games 'out-of-the-box' (i.e. without no-cd patch).

But be warned : linus hates changing values that worked for years by some "magical" ones.

Revision history for this message
Igbaäl (cerebro-alexiel) wrote :

No answer from Dan so I filed a bug @ https://bugzilla.kernel.org/show_bug.cgi?id=39062
I tried to make it clear but it ends up being a bit lengthy.

Revision history for this message
Dan Kegel (dank) wrote :

I didn't answer because Scott had asked the same question and I had already answered it, and I was hoping you would notice.
Sorry for being lazy.

Revision history for this message
Brad Figg (brad-figg) wrote : Missing required logs.

This bug is missing log files that will aid in dianosing the problem. From a terminal window please run:

apport-collect 777528

and then change the status of the bug back to 'New'.

If, due to the nature of the issue you have encountered, you are unable to run this command, please add a comment stating that fact and change the bug status to 'Confirmed'.

This change has been made by an automated script, maintained by the Ubuntu Kernel Team.

Changed in linux (Ubuntu):
status: New → Incomplete
Revision history for this message
madbiologist (me-again) wrote :

The problem is known and fixed upstream. Log files not necessary.

Changed in linux (Ubuntu):
status: Incomplete → New
Revision history for this message
madbiologist (me-again) wrote :

It looks like this fix will be included upstream in the upcoming 3.1 kernel:

author Andrew Morton <email address hidden>
Tue, 26 Jul 2011 10:14:49 +0000 (20:14 +1000)
committer Stephen Rothwell <email address hidden>
Fri, 29 Jul 2011 05:49:13 +0000 (15:49 +1000)
commit 4708e0b00cc54585fa752071a3164131f00a023d
tree b2f84d8fb532fd34620b48affc7717e831ff2f23
parent feb694144c51b300480b10267d63e4f4eb3927ff

The report has an ISO which has a very long manufacturer ID. It seems
that Linux is wrong, not the ISO maker.

Relax the check for the length of this field: emit a warning and truncate
the incoming data to 2048 bytes rather than rejecting the entire thing.

dvd_manufact.value isn't null-terminated. I'm not even sure if it's a
string. The kernel doesn't apepar to use it anyway.

Addresses https://bugzilla.kernel.org/show_bug.cgi?id=39062

Reported-by:
Tested-by:
Cc: Jens Axboe
Signed-off-by: Andrew Morton

Changed in linux (Ubuntu):
status: New → Fix Committed
Revision history for this message
Igbaäl (cerebro-alexiel) wrote :

Now the patch is written, tested, signed-off and will be merged soon, I'd like to ask ubuntu kernel maintainers if it would be possible to backport the commit [1].

The change is minor and would help lots of people who need illegal stuff to play their favorite game without the CD.
Wine need patching but I'm working on writing a clean patch based on Steven Wallace work.

Thanks
--
[1] http://git.cmpxchg.org/?p=linux-mmotm.git;a=commitdiff;h=a9f517d381acef071aebdfd33510b9d29c1b413b

Revision history for this message
Igbaäl (cerebro-alexiel) wrote :

FYI, it has been committed in the linus tree @ http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=commitdiff;h=aec9f377e4f235c47e27fd8a429555dfa2dda342.

Will be part of the final 3.1 kernel.

Revision history for this message
Dan Kegel (dank) wrote :

Awesome! Thanks for filing that upstream bug.

I hope this makes it into ubuntu 11.10.

Changed in linux:
importance: Unknown → Medium
status: Unknown → Fix Released
Revision history for this message
Julian Wiedmann (jwiedmann) wrote :

Closing per comment #11.

Changed in linux (Ubuntu):
status: Fix Committed → Fix Released
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.