[PATCH] cdrom_sysctl_info fix

[PATCH] cdrom_sysctl_info fix

[Dave Young]

Hi,

Fix the cdrom_sysctl_info possible buffer overwrite bug. Somd codingstyle fixes are included as well. 

diff based on 2.6.22-rc4

Signed-off-by: Dave Young <hidave.darkstar@gmail.com>
---
drivers/cdrom/cdrom.c | 186 +-
1 files changed, 102 insertions(+), 84 deletions(-)

diff -upr linux/drivers/cdrom/cdrom.c linux.new/drivers/cdrom/cdrom.c
--- linux/drivers/cdrom/cdrom.c	2007-06-14 14:05:04.000000000 +0000
+++ linux.new/drivers/cdrom/cdrom.c	2007-06-14 14:11:58.000000000 +0000
@@ -3290,102 +3290,120 @@ static struct cdrom_sysctl_settings {
 } cdrom_sysctl_settings;
 
 static int cdrom_sysctl_info(ctl_table *ctl, int write, struct file * filp,
-                           void __user *buffer, size_t *lenp, loff_t *ppos)
+				void __user *buffer, size_t *lenp, loff_t *ppos)
 {
-        int pos;
+	int pos;
 	struct cdrom_device_info *cdi;
 	char *info = cdrom_sysctl_settings.info;
+	int size = sizeof(cdrom_sysctl_settings.info);
 	
 	if (!*lenp || (*ppos && !write)) {
 		*lenp = 0;
 		return 0;
 	}
 
-	pos = sprintf(info, "CD-ROM information, " VERSION "\n");
+	pos = scnprintf(info, size, "CD-ROM information, " VERSION "\n");
 	
-	pos += sprintf(info+pos, "\ndrive name:\t");
-	for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
-	    pos += sprintf(info+pos, "\t%s", cdi->name);
-
-	pos += sprintf(info+pos, "\ndrive speed:\t");
-	for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
-	    pos += sprintf(info+pos, "\t%d", cdi->speed);
-
-	pos += sprintf(info+pos, "\ndrive # of slots:");
-	for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
-	    pos += sprintf(info+pos, "\t%d", cdi->capacity);
-
-	pos += sprintf(info+pos, "\nCan close tray:\t");
-	for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
-	    pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_CLOSE_TRAY) != 0);
-
-	pos += sprintf(info+pos, "\nCan open tray:\t");
-	for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
-	    pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_OPEN_TRAY) != 0);
-
-	pos += sprintf(info+pos, "\nCan lock tray:\t");
-	for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
-	    pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_LOCK) != 0);
-
-	pos += sprintf(info+pos, "\nCan change speed:");
-	for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
-	    pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_SELECT_SPEED) != 0);
-
-	pos += sprintf(info+pos, "\nCan select disk:");
-	for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
-	    pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_SELECT_DISC) != 0);
-
-	pos += sprintf(info+pos, "\nCan read multisession:");
-	for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
-	    pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_MULTI_SESSION) != 0);
-
-	pos += sprintf(info+pos, "\nCan read MCN:\t");
-	for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
-	    pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_MCN) != 0);
-
-	pos += sprintf(info+pos, "\nReports media changed:");
-	for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
-	    pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_MEDIA_CHANGED) != 0);
-
-	pos += sprintf(info+pos, "\nCan play audio:\t");
-	for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
-	    pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_PLAY_AUDIO) != 0);
-
-	pos += sprintf(info+pos, "\nCan write CD-R:\t");
-	for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
-	    pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_CD_R) != 0);
-
-	pos += sprintf(info+pos, "\nCan write CD-RW:");
-	for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
-	    pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_CD_RW) != 0);
-
-	pos += sprintf(info+pos, "\nCan read DVD:\t");
-	for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
-	    pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_DVD) != 0);
-
-	pos += sprintf(info+pos, "\nCan write DVD-R:");
-	for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
-	    pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_DVD_R) != 0);
-
-	pos += sprintf(info+pos, "\nCan write DVD-RAM:");
-	for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
-	    pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_DVD_RAM) != 0);
-
-	pos += sprintf(info+pos, "\nCan read MRW:\t");
-	for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
-	    pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_MRW) != 0);
-
-	pos += sprintf(info+pos, "\nCan write MRW:\t");
-	for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
-	    pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_MRW_W) != 0);
-
-	pos += sprintf(info+pos, "\nCan write RAM:\t");
-	for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
-	    pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_RAM) != 0);
+	pos += scnprintf(info + pos, size - pos, "\ndrive name:\t");
+	for (cdi = topCdromPtr; cdi != NULL; cdi = cdi->next)
+		pos += scnprintf(info + pos, size - pos, "\t%s", cdi->name);
+
+	pos += scnprintf(info + pos, size - pos ,"\ndrive speed:\t");
+	for (cdi = topCdromPtr; cdi != NULL; cdi = cdi->next)
+		pos += scnprintf(info + pos, size - pos, "\t%d", cdi->speed);
+
+	pos += scnprintf(info + pos, size - pos, "\ndrive # of slots:");
+	for (cdi = topCdromPtr; cdi != NULL; cdi = cdi->next)
+		pos += scnprintf(info + pos, size - pos, "\t%d", cdi->capacity);
+
+	pos += scnprintf(info + pos, size - pos, "\nCan close tray:\t");
+	for (cdi = topCdromPtr; cdi != NULL; cdi = cdi->next)
+		pos += scnprintf(info + pos, size - pos, "\t%d", 
+					CDROM_CAN(CDC_CLOSE_TRAY) != 0);
+
+	pos += scnprintf(info + pos, size - pos, "\nCan open tray:\t");
+	for (cdi = topCdromPtr; cdi != NULL; cdi = cdi->next)
+		pos += scnprintf(info + pos, size - pos, "\t%d", 
+					CDROM_CAN(CDC_OPEN_TRAY) != 0);
+
+	pos += scnprintf(info + pos, size - pos, "\nCan lock tray:\t");
+	for (cdi = topCdromPtr; cdi != NULL; cdi = cdi->next)
+		pos += scnprintf(info + pos, size - pos, "\t%d", 
+					CDROM_CAN(CDC_LOCK) != 0);
+
+	pos += scnprintf(info + pos, size - pos, "\nCan change speed:");
+	for (cdi = topCdromPtr; cdi != NULL; cdi = cdi->next)
+		pos += scnprintf(info + pos, size - pos, "\t%d", 
+					CDROM_CAN(CDC_SELECT_SPEED) != 0);
+
+	pos += scnprintf(info + pos, size - pos, "\nCan select disk:");
+	for (cdi = topCdromPtr; cdi != NULL; cdi = cdi->next)
+		pos += scnprintf(info + pos, size - pos, "\t%d", 
+					CDROM_CAN(CDC_SELECT_DISC) != 0);
+
+	pos += scnprintf(info + pos, size - pos, "\nCan read multisession:");
+	for (cdi = topCdromPtr; cdi != NULL; cdi = cdi->next)
+		pos += scnprintf(info + pos, size - pos, "\t%d", 
+					CDROM_CAN(CDC_MULTI_SESSION) != 0);
+
+	pos += scnprintf(info + pos, size - pos, "\nCan read MCN:\t");
+	for (cdi = topCdromPtr; cdi != NULL; cdi = cdi->next)
+		pos += scnprintf(info + pos, size - pos, "\t%d", 
+					CDROM_CAN(CDC_MCN) != 0);
+
+	pos += scnprintf(info + pos, size - pos, "\nReports media changed:");
+	for (cdi = topCdromPtr; cdi != NULL; cdi = cdi->next)
+		pos += scnprintf(info + pos, size - pos, "\t%d", 
+					CDROM_CAN(CDC_MEDIA_CHANGED) != 0);
+
+	pos += scnprintf(info + pos, size - pos, "\nCan play audio:\t");
+	for (cdi = topCdromPtr; cdi != NULL; cdi = cdi->next)
+		pos += scnprintf(info + pos, size - pos, "\t%d", 
+					CDROM_CAN(CDC_PLAY_AUDIO) != 0);
+
+	pos += scnprintf(info + pos, size - pos, "\nCan write CD-R:\t");
+	for (cdi = topCdromPtr; cdi != NULL; cdi = cdi->next)
+		pos += scnprintf(info + pos, size - pos, "\t%d", 
+					CDROM_CAN(CDC_CD_R) != 0);
+
+	pos += scnprintf(info + pos, size - pos, "\nCan write CD-RW:");
+	for (cdi = topCdromPtr; cdi != NULL; cdi = cdi->next)
+		pos += scnprintf(info + pos, size - pos, "\t%d", 
+					CDROM_CAN(CDC_CD_RW) != 0);
+
+	pos += scnprintf(info + pos, size - pos, "\nCan read DVD:\t");
+	for (cdi = topCdromPtr; cdi != NULL; cdi = cdi->next)
+		pos += scnprintf(info + pos, size - pos, "\t%d", 
+					CDROM_CAN(CDC_DVD) != 0);
+
+	pos += scnprintf(info + pos, size - pos, "\nCan write DVD-R:");
+	for (cdi = topCdromPtr; cdi != NULL; cdi = cdi->next)
+		pos += scnprintf(info + pos, size - pos, "\t%d", 
+					CDROM_CAN(CDC_DVD_R) != 0);
+
+	pos += scnprintf(info + pos, size - pos, "\nCan write DVD-RAM:");
+	for (cdi = topCdromPtr; cdi != NULL; cdi = cdi->next)
+		pos += scnprintf(info + pos, size - pos, "\t%d", 
+					CDROM_CAN(CDC_DVD_RAM) != 0);
+
+	pos += scnprintf(info + pos, size - pos, "\nCan read MRW:\t");
+	for (cdi = topCdromPtr; cdi != NULL; cdi = cdi->next)
+		pos += scnprintf(info + pos, size - pos, "\t%d", 
+					CDROM_CAN(CDC_MRW) != 0);
+
+	pos += scnprintf(info + pos, size - pos, "\nCan write MRW:\t");
+	for (cdi = topCdromPtr; cdi != NULL; cdi = cdi->next)
+		pos += scnprintf(info + pos, size - pos, "\t%d", 
+					CDROM_CAN(CDC_MRW_W) != 0);
+
+	pos += scnprintf(info + pos, size - pos, "\nCan write RAM:\t");
+	for (cdi = topCdromPtr; cdi != NULL; cdi = cdi->next)
+		pos += scnprintf(info + pos, size - pos, "\t%d", 
+					CDROM_CAN(CDC_RAM) != 0);
 
-	strcpy(info+pos,"\n\n");
+	scnprintf(info + pos, size - pos, "\n\n");
 		
-        return proc_dostring(ctl, write, filp, buffer, lenp, ppos);
+	return proc_dostring(ctl, write, filp, buffer, lenp, ppos);
 }
 
 /* Unfortunately, per device settings are not implemented through

Regards
dave

Re: [PATCH] cdrom_sysctl_info fix

[dave young]

Hi Andrew,

Sorry for reply to myself,  does the Jens Axboe's email is outdated?
which one is the latest?

And  Jens, could you please update your email address?

Re: [PATCH] cdrom_sysctl_info fix

[Randy Dunlap]

On Thu, 14 Jun 2007 06:40:49 +0000 dave young wrote:

> Hi Andrew,
> 
> Sorry for reply to myself,  does the Jens Axboe's email is outdated?
> which one is the latest?
> 
> And  Jens, could you please update your email address?

Better to use the email address in the MAINTAINERS file than
the one in the driver source file.

---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

Re: [PATCH] cdrom_sysctl_info fix

[dave young]

Hi,
> Better to use the email address in the MAINTAINERS file than
> the one in the driver source file.

Really? I searched the list, found axboe use the address
jens.axboe@oracle.com, same as what andrew said. does the MAINTAINERS
file be updated?

Regards
dave

Re: [PATCH] cdrom_sysctl_info fix

[Randy Dunlap]

dave young wrote:
> Hi,
>> Better to use the email address in the MAINTAINERS file than
>> the one in the driver source file.
> 
> Really? I searched the list, found axboe use the address
> jens.axboe@oracle.com, same as what andrew said. does the MAINTAINERS
> file be updated?

Could be, but that's up to Jens.

-- 
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

Re: [PATCH] cdrom_sysctl_info fix

[Jens Axboe]

On Fri, Jun 15 2007, dave young wrote:
> Hi,
> >Better to use the email address in the MAINTAINERS file than
> >the one in the driver source file.
> 
> Really? I searched the list, found axboe use the address
> jens.axboe@oracle.com, same as what andrew said. does the MAINTAINERS
> file be updated?

Hey, no point in CC'ing me twice! axboe@kernel.dk works just fine as
well.

-- 
Jens Axboe

Re: [PATCH] cdrom_sysctl_info fix

[Dave Young]

Hi,
>On Fri, Jun 15, 2007 at 07:58:47AM +0200, Jens Axboe wrote:
> Hey, no point in CC'ing me twice! axboe@kernel.dk works just fine as
> well.
> 
I'm sorry for it , jens. Now I resend the patch after remove some checkpatch.pl warnings(line breaking trailing space). 

If user have many cdrom drives, then the buffer is not enough, so I write this patch along with a little coding style fixing. Do you think I need to rewrite to two patches?

Regards
dave
----------------------

From: Dave Young <hidave.darkstar@gmail.com>

cdrom_sysctl_info may cause buffer overwrite.

Signed-off-by: Dave Young <hidave.darkstar@gmail.com>
---
diff -upr linux/drivers/cdrom/cdrom.c linux.new/drivers/cdrom/cdrom.c
--- linux/drivers/cdrom/cdrom.c	2007-06-14 14:05:04.000000000 +0000
+++ linux.new/drivers/cdrom/cdrom.c	2007-06-15 13:24:10.000000000 +0000
@@ -3290,102 +3290,120 @@ static struct cdrom_sysctl_settings {
 } cdrom_sysctl_settings;
 
 static int cdrom_sysctl_info(ctl_table *ctl, int write, struct file * filp,
-                           void __user *buffer, size_t *lenp, loff_t *ppos)
+				void __user *buffer, size_t *lenp, loff_t *ppos)
 {
-        int pos;
+	int pos;
 	struct cdrom_device_info *cdi;
 	char *info = cdrom_sysctl_settings.info;
+	int size = sizeof(cdrom_sysctl_settings.info);
 	
 	if (!*lenp || (*ppos && !write)) {
 		*lenp = 0;
 		return 0;
 	}
 
-	pos = sprintf(info, "CD-ROM information, " VERSION "\n");
+	pos = scnprintf(info, size, "CD-ROM information, " VERSION "\n");
 	
-	pos += sprintf(info+pos, "\ndrive name:\t");
-	for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
-	    pos += sprintf(info+pos, "\t%s", cdi->name);
-
-	pos += sprintf(info+pos, "\ndrive speed:\t");
-	for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
-	    pos += sprintf(info+pos, "\t%d", cdi->speed);
-
-	pos += sprintf(info+pos, "\ndrive # of slots:");
-	for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
-	    pos += sprintf(info+pos, "\t%d", cdi->capacity);
-
-	pos += sprintf(info+pos, "\nCan close tray:\t");
-	for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
-	    pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_CLOSE_TRAY) != 0);
-
-	pos += sprintf(info+pos, "\nCan open tray:\t");
-	for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
-	    pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_OPEN_TRAY) != 0);
-
-	pos += sprintf(info+pos, "\nCan lock tray:\t");
-	for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
-	    pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_LOCK) != 0);
-
-	pos += sprintf(info+pos, "\nCan change speed:");
-	for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
-	    pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_SELECT_SPEED) != 0);
-
-	pos += sprintf(info+pos, "\nCan select disk:");
-	for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
-	    pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_SELECT_DISC) != 0);
-
-	pos += sprintf(info+pos, "\nCan read multisession:");
-	for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
-	    pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_MULTI_SESSION) != 0);
-
-	pos += sprintf(info+pos, "\nCan read MCN:\t");
-	for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
-	    pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_MCN) != 0);
-
-	pos += sprintf(info+pos, "\nReports media changed:");
-	for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
-	    pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_MEDIA_CHANGED) != 0);
-
-	pos += sprintf(info+pos, "\nCan play audio:\t");
-	for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
-	    pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_PLAY_AUDIO) != 0);
-
-	pos += sprintf(info+pos, "\nCan write CD-R:\t");
-	for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
-	    pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_CD_R) != 0);
-
-	pos += sprintf(info+pos, "\nCan write CD-RW:");
-	for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
-	    pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_CD_RW) != 0);
-
-	pos += sprintf(info+pos, "\nCan read DVD:\t");
-	for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
-	    pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_DVD) != 0);
-
-	pos += sprintf(info+pos, "\nCan write DVD-R:");
-	for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
-	    pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_DVD_R) != 0);
-
-	pos += sprintf(info+pos, "\nCan write DVD-RAM:");
-	for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
-	    pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_DVD_RAM) != 0);
-
-	pos += sprintf(info+pos, "\nCan read MRW:\t");
-	for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
-	    pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_MRW) != 0);
-
-	pos += sprintf(info+pos, "\nCan write MRW:\t");
-	for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
-	    pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_MRW_W) != 0);
-
-	pos += sprintf(info+pos, "\nCan write RAM:\t");
-	for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
-	    pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_RAM) != 0);
+	pos += scnprintf(info + pos, size - pos, "\ndrive name:\t");
+	for (cdi = topCdromPtr; cdi != NULL; cdi = cdi->next)
+		pos += scnprintf(info + pos, size - pos, "\t%s", cdi->name);
+
+	pos += scnprintf(info + pos, size - pos, "\ndrive speed:\t");
+	for (cdi = topCdromPtr; cdi != NULL; cdi = cdi->next)
+		pos += scnprintf(info + pos, size - pos, "\t%d", cdi->speed);
+
+	pos += scnprintf(info + pos, size - pos, "\ndrive # of slots:");
+	for (cdi = topCdromPtr; cdi != NULL; cdi = cdi->next)
+		pos += scnprintf(info + pos, size - pos, "\t%d", cdi->capacity);
+
+	pos += scnprintf(info + pos, size - pos, "\nCan close tray:\t");
+	for (cdi = topCdromPtr; cdi != NULL; cdi = cdi->next)
+		pos += scnprintf(info + pos, size - pos, "\t%d",
+					CDROM_CAN(CDC_CLOSE_TRAY) != 0);
+
+	pos += scnprintf(info + pos, size - pos, "\nCan open tray:\t");
+	for (cdi = topCdromPtr; cdi != NULL; cdi = cdi->next)
+		pos += scnprintf(info + pos, size - pos, "\t%d",
+					CDROM_CAN(CDC_OPEN_TRAY) != 0);
+
+	pos += scnprintf(info + pos, size - pos, "\nCan lock tray:\t");
+	for (cdi = topCdromPtr; cdi != NULL; cdi = cdi->next)
+		pos += scnprintf(info + pos, size - pos, "\t%d",
+					CDROM_CAN(CDC_LOCK) != 0);
+
+	pos += scnprintf(info + pos, size - pos, "\nCan change speed:");
+	for (cdi = topCdromPtr; cdi != NULL; cdi = cdi->next)
+		pos += scnprintf(info + pos, size - pos, "\t%d",
+					CDROM_CAN(CDC_SELECT_SPEED) != 0);
+
+	pos += scnprintf(info + pos, size - pos, "\nCan select disk:");
+	for (cdi = topCdromPtr; cdi != NULL; cdi = cdi->next)
+		pos += scnprintf(info + pos, size - pos, "\t%d",
+					CDROM_CAN(CDC_SELECT_DISC) != 0);
+
+	pos += scnprintf(info + pos, size - pos, "\nCan read multisession:");
+	for (cdi = topCdromPtr; cdi != NULL; cdi = cdi->next)
+		pos += scnprintf(info + pos, size - pos, "\t%d",
+					CDROM_CAN(CDC_MULTI_SESSION) != 0);
+
+	pos += scnprintf(info + pos, size - pos, "\nCan read MCN:\t");
+	for (cdi = topCdromPtr; cdi != NULL; cdi = cdi->next)
+		pos += scnprintf(info + pos, size - pos, "\t%d",
+					CDROM_CAN(CDC_MCN) != 0);
+
+	pos += scnprintf(info + pos, size - pos, "\nReports media changed:");
+	for (cdi = topCdromPtr; cdi != NULL; cdi = cdi->next)
+		pos += scnprintf(info + pos, size - pos, "\t%d",
+					CDROM_CAN(CDC_MEDIA_CHANGED) != 0);
+
+	pos += scnprintf(info + pos, size - pos, "\nCan play audio:\t");
+	for (cdi = topCdromPtr; cdi != NULL; cdi = cdi->next)
+		pos += scnprintf(info + pos, size - pos, "\t%d",
+					CDROM_CAN(CDC_PLAY_AUDIO) != 0);
+
+	pos += scnprintf(info + pos, size - pos, "\nCan write CD-R:\t");
+	for (cdi = topCdromPtr; cdi != NULL; cdi = cdi->next)
+		pos += scnprintf(info + pos, size - pos, "\t%d",
+					CDROM_CAN(CDC_CD_R) != 0);
+
+	pos += scnprintf(info + pos, size - pos, "\nCan write CD-RW:");
+	for (cdi = topCdromPtr; cdi != NULL; cdi = cdi->next)
+		pos += scnprintf(info + pos, size - pos, "\t%d",
+					CDROM_CAN(CDC_CD_RW) != 0);
+
+	pos += scnprintf(info + pos, size - pos, "\nCan read DVD:\t");
+	for (cdi = topCdromPtr; cdi != NULL; cdi = cdi->next)
+		pos += scnprintf(info + pos, size - pos, "\t%d",
+					CDROM_CAN(CDC_DVD) != 0);
+
+	pos += scnprintf(info + pos, size - pos, "\nCan write DVD-R:");
+	for (cdi = topCdromPtr; cdi != NULL; cdi = cdi->next)
+		pos += scnprintf(info + pos, size - pos, "\t%d",
+					CDROM_CAN(CDC_DVD_R) != 0);
+
+	pos += scnprintf(info + pos, size - pos, "\nCan write DVD-RAM:");
+	for (cdi = topCdromPtr; cdi != NULL; cdi = cdi->next)
+		pos += scnprintf(info + pos, size - pos, "\t%d",
+					CDROM_CAN(CDC_DVD_RAM) != 0);
+
+	pos += scnprintf(info + pos, size - pos, "\nCan read MRW:\t");
+	for (cdi = topCdromPtr; cdi != NULL; cdi = cdi->next)
+		pos += scnprintf(info + pos, size - pos, "\t%d",
+					CDROM_CAN(CDC_MRW) != 0);
+
+	pos += scnprintf(info + pos, size - pos, "\nCan write MRW:\t");
+	for (cdi = topCdromPtr; cdi != NULL; cdi = cdi->next)
+		pos += scnprintf(info + pos, size - pos, "\t%d",
+					CDROM_CAN(CDC_MRW_W) != 0);
+
+	pos += scnprintf(info + pos, size - pos, "\nCan write RAM:\t");
+	for (cdi = topCdromPtr; cdi != NULL; cdi = cdi->next)
+		pos += scnprintf(info + pos, size - pos, "\t%d",
+					CDROM_CAN(CDC_RAM) != 0);
 
-	strcpy(info+pos,"\n\n");
+	scnprintf(info + pos, size - pos, "\n\n");
 		
-        return proc_dostring(ctl, write, filp, buffer, lenp, ppos);
+	return proc_dostring(ctl, write, filp, buffer, lenp, ppos);
 }
 
 /* Unfortunately, per device settings are not implemented through

Re: [PATCH] cdrom_sysctl_info fix

[Jens Axboe]

On Thu, Jun 14 2007, Dave Young wrote:
> Hi,
> 
> Fix the cdrom_sysctl_info possible buffer overwrite bug. Somd
> codingstyle fixes are included as well. 

How about something like this? The current code is actually racy,
because there's no protection against adding/removing a cdrom while it
runs. This patch is largely based on yours, just abstracted the printing
and looping into a function and bail when we run out of room (and print
a message to that effect).

If you could test it and verify that it does the right thing (eg prints
the same as before), that would be great!

diff --git a/drivers/cdrom/cdrom.c b/drivers/cdrom/cdrom.c
index 3625a05..ae5a475 100644
--- a/drivers/cdrom/cdrom.c
+++ b/drivers/cdrom/cdrom.c
@@ -302,7 +302,7 @@ module_param(lockdoor, bool, 0);
 module_param(check_media_type, bool, 0);
 module_param(mrw_format_restart, bool, 0);
 
-static DEFINE_SPINLOCK(cdrom_lock);
+static DEFINE_MUTEX(cdrom_mutex);
 
 static const char *mrw_format_status[] = {
 	"not mrw",
@@ -438,10 +438,10 @@ int register_cdrom(struct cdrom_device_info *cdi)
 		cdo->generic_packet = cdrom_dummy_generic_packet;
 
 	cdinfo(CD_REG_UNREG, "drive \"/dev/%s\" registered\n", cdi->name);
-	spin_lock(&cdrom_lock);
+	mutex_lock(&cdrom_mutex);
 	cdi->next = topCdromPtr; 	
 	topCdromPtr = cdi;
-	spin_unlock(&cdrom_lock);
+	mutex_unlock(&cdrom_mutex);
 	return 0;
 }
 #undef ENSURE
@@ -452,7 +452,7 @@ int unregister_cdrom(struct cdrom_device_info *unreg)
 	cdinfo(CD_OPEN, "entering unregister_cdrom\n"); 
 
 	prev = NULL;
-	spin_lock(&cdrom_lock);
+	mutex_lock(&cdrom_mutex);
 	cdi = topCdromPtr;
 	while (cdi && cdi != unreg) {
 		prev = cdi;
@@ -460,7 +460,7 @@ int unregister_cdrom(struct cdrom_device_info *unreg)
 	}
 
 	if (cdi == NULL) {
-		spin_unlock(&cdrom_lock);
+		mutex_unlock(&cdrom_mutex);
 		return -2;
 	}
 	if (prev)
@@ -468,7 +468,7 @@ int unregister_cdrom(struct cdrom_device_info *unreg)
 	else
 		topCdromPtr = cdi->next;
 
-	spin_unlock(&cdrom_lock);
+	mutex_unlock(&cdrom_mutex);
 
 	if (cdi->exit)
 		cdi->exit(cdi);
@@ -3289,103 +3289,112 @@ static struct cdrom_sysctl_settings {
 	int	check;			/* check media type */
 } cdrom_sysctl_settings;
 
+static int cdrom_print_int(const char *header, int val, char *info, int *pos)
+{
+	const int max_size = sizeof(cdrom_sysctl_settings.info);
+	struct cdrom_device_info *cdi;
+	int ret;
+
+	ret = scnprintf(info + *pos, max_size, header);
+	if (!ret)
+		return 1;
+
+	*pos += ret;
+
+	for (cdi = topCdromPtr; cdi; cdi = cdi->next) {
+		ret = scnprintf(info + *pos, max_size, "\t%d", val);
+		if (!ret)
+			return 1;
+		*pos += ret;
+	}
+
+	return 0;
+}
+
 static int cdrom_sysctl_info(ctl_table *ctl, int write, struct file * filp,
                            void __user *buffer, size_t *lenp, loff_t *ppos)
 {
-        int pos;
+	int pos;
 	struct cdrom_device_info *cdi;
 	char *info = cdrom_sysctl_settings.info;
+	const int max_size = sizeof(cdrom_sysctl_settings.info);
 	
 	if (!*lenp || (*ppos && !write)) {
 		*lenp = 0;
 		return 0;
 	}
 
+	mutex_lock(&cdrom_mutex);
+
 	pos = sprintf(info, "CD-ROM information, " VERSION "\n");
 	
-	pos += sprintf(info+pos, "\ndrive name:\t");
-	for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
-	    pos += sprintf(info+pos, "\t%s", cdi->name);
-
-	pos += sprintf(info+pos, "\ndrive speed:\t");
-	for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
-	    pos += sprintf(info+pos, "\t%d", cdi->speed);
-
-	pos += sprintf(info+pos, "\ndrive # of slots:");
-	for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
-	    pos += sprintf(info+pos, "\t%d", cdi->capacity);
-
-	pos += sprintf(info+pos, "\nCan close tray:\t");
-	for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
-	    pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_CLOSE_TRAY) != 0);
-
-	pos += sprintf(info+pos, "\nCan open tray:\t");
-	for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
-	    pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_OPEN_TRAY) != 0);
-
-	pos += sprintf(info+pos, "\nCan lock tray:\t");
-	for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
-	    pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_LOCK) != 0);
-
-	pos += sprintf(info+pos, "\nCan change speed:");
-	for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
-	    pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_SELECT_SPEED) != 0);
-
-	pos += sprintf(info+pos, "\nCan select disk:");
-	for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
-	    pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_SELECT_DISC) != 0);
-
-	pos += sprintf(info+pos, "\nCan read multisession:");
-	for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
-	    pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_MULTI_SESSION) != 0);
-
-	pos += sprintf(info+pos, "\nCan read MCN:\t");
-	for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
-	    pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_MCN) != 0);
-
-	pos += sprintf(info+pos, "\nReports media changed:");
-	for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
-	    pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_MEDIA_CHANGED) != 0);
-
-	pos += sprintf(info+pos, "\nCan play audio:\t");
-	for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
-	    pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_PLAY_AUDIO) != 0);
-
-	pos += sprintf(info+pos, "\nCan write CD-R:\t");
-	for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
-	    pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_CD_R) != 0);
-
-	pos += sprintf(info+pos, "\nCan write CD-RW:");
-	for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
-	    pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_CD_RW) != 0);
-
-	pos += sprintf(info+pos, "\nCan read DVD:\t");
-	for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
-	    pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_DVD) != 0);
-
-	pos += sprintf(info+pos, "\nCan write DVD-R:");
-	for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
-	    pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_DVD_R) != 0);
-
-	pos += sprintf(info+pos, "\nCan write DVD-RAM:");
-	for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
-	    pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_DVD_RAM) != 0);
-
-	pos += sprintf(info+pos, "\nCan read MRW:\t");
-	for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
-	    pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_MRW) != 0);
-
-	pos += sprintf(info+pos, "\nCan write MRW:\t");
-	for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
-	    pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_MRW_W) != 0);
-
-	pos += sprintf(info+pos, "\nCan write RAM:\t");
-	for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
-	    pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_RAM) != 0);
-
-	strcpy(info+pos,"\n\n");
-		
-        return proc_dostring(ctl, write, filp, buffer, lenp, ppos);
+	pos += scnprintf(info + pos, max_size - pos, "\ndrive name:\t");
+	for (cdi = topCdromPtr; cdi != NULL; cdi = cdi->next)
+		pos += scnprintf(info + pos, max_size - pos, "\t%s", cdi->name);
+
+	if (cdrom_print_int("\ndrive speed:\t", cdi->speed, info, &pos))
+		goto done;
+	if (cdrom_print_int("\ndrive # of slots:", cdi->capacity, info, &pos))
+		goto done;
+	if (cdrom_print_int("\nCan close tray:\t",
+				CDROM_CAN(CDC_CLOSE_TRAY) != 0, info, &pos))
+		goto done;
+	if (cdrom_print_int("\nCan open tray:\t",
+				CDROM_CAN(CDC_OPEN_TRAY) != 0, info, &pos))
+		goto done;
+	if (cdrom_print_int("\nCan lock tray:\t",
+				CDROM_CAN(CDC_LOCK) != 0, info, &pos))
+		goto done;
+	if (cdrom_print_int("\nCan change speed:\t",
+				CDROM_CAN(CDC_SELECT_SPEED) != 0, info, &pos))
+		goto done;
+	if (cdrom_print_int("\nCan select disk:\t",
+				CDROM_CAN(CDC_SELECT_DISC) != 0, info, &pos))
+		goto done;
+	if (cdrom_print_int("\nCan read multisession:",
+				CDROM_CAN(CDC_MULTI_SESSION) != 0, info, &pos))
+		goto done;
+	if (cdrom_print_int("\nCan read MCN:",
+				CDROM_CAN(CDC_MCN) != 0, info, &pos))
+		goto done;
+	if (cdrom_print_int("\nReports media changed:",
+				CDROM_CAN(CDC_MEDIA_CHANGED) != 0, info, &pos))
+		goto done;
+	if (cdrom_print_int("\nCan play audio:",
+				CDROM_CAN(CDC_PLAY_AUDIO) != 0, info, &pos))
+		goto done;
+	if (cdrom_print_int("\nCan write CD-R:\t",
+				CDROM_CAN(CDC_CD_R) != 0, info, &pos))
+		goto done;
+	if (cdrom_print_int("\nCan write CD-RW:\t",
+				CDROM_CAN(CDC_CD_RW) != 0, info, &pos))
+		goto done;
+	if (cdrom_print_int("\nCan read DVD:\t",
+				CDROM_CAN(CDC_DVD) != 0, info, &pos))
+		goto done;
+	if (cdrom_print_int("\nCan write DVD-R:\t",
+				CDROM_CAN(CDC_DVD_R) != 0, info, &pos))
+		goto done;
+	if (cdrom_print_int("\nCan write DVD-RAM:\t",
+				CDROM_CAN(CDC_DVD_RAM) != 0, info, &pos))
+		goto done;
+	if (cdrom_print_int("\nCan read MRW:\t",
+				CDROM_CAN(CDC_MRW) != 0, info, &pos))
+		goto done;
+	if (cdrom_print_int("\nCan write MRW:\t",
+				CDROM_CAN(CDC_MRW_W) != 0, info, &pos))
+		goto done;
+	if (cdrom_print_int("\nCan write RAM:\t",
+				CDROM_CAN(CDC_RAM) != 0, info, &pos))
+		goto done;
+	if (!scnprintf(info + pos, max_size - pos, "\n\n"))
+		goto done;
+doit:
+	mutex_unlock(&cdrom_mutex);
+	return proc_dostring(ctl, write, filp, buffer, lenp, ppos);
+done:
+	printk(KERN_INFO "cdrom: info buffer too small\n");
+	goto doit;
 }
 
 /* Unfortunately, per device settings are not implemented through

-- 
Jens Axboe

Re: [PATCH] cdrom_sysctl_info fix

[Dave Young]

Hi jens,
Thanks for rework this patch.But it has some bugs, results kernel oops.

On Fri, Jun 15, 2007 at 03:26:57PM +0200, Jens Axboe wrote:
> On Thu, Jun 14 2007, Dave Young wrote:
> > Hi,
> > 
> > Fix the cdrom_sysctl_info possible buffer overwrite bug. Somd
> > codingstyle fixes are included as well. 
> 
> How about something like this? The current code is actually racy,
> because there's no protection against adding/removing a cdrom while it
> runs. This patch is largely based on yours, just abstracted the printing
> and looping into a function and bail when we run out of room (and print
> a message to that effect).
You are right
> 
> If you could test it and verify that it does the right thing (eg prints
> the same as before), that would be great!
> 
> diff --git a/drivers/cdrom/cdrom.c b/drivers/cdrom/cdrom.c
> index 3625a05..ae5a475 100644
> --- a/drivers/cdrom/cdrom.c
> +++ b/drivers/cdrom/cdrom.c
> @@ -302,7 +302,7 @@ module_param(lockdoor, bool, 0);
>  module_param(check_media_type, bool, 0);
>  module_param(mrw_format_restart, bool, 0);
>  
> -static DEFINE_SPINLOCK(cdrom_lock);
> +static DEFINE_MUTEX(cdrom_mutex);
>  
>  static const char *mrw_format_status[] = {
>  	"not mrw",
> @@ -438,10 +438,10 @@ int register_cdrom(struct cdrom_device_info *cdi)
>  		cdo->generic_packet = cdrom_dummy_generic_packet;
>  
>  	cdinfo(CD_REG_UNREG, "drive \"/dev/%s\" registered\n", cdi->name);
> -	spin_lock(&cdrom_lock);
> +	mutex_lock(&cdrom_mutex);
>  	cdi->next = topCdromPtr; 	
>  	topCdromPtr = cdi;
> -	spin_unlock(&cdrom_lock);
> +	mutex_unlock(&cdrom_mutex);
>  	return 0;
>  }
>  #undef ENSURE
> @@ -452,7 +452,7 @@ int unregister_cdrom(struct cdrom_device_info *unreg)
>  	cdinfo(CD_OPEN, "entering unregister_cdrom\n"); 
>  
>  	prev = NULL;
> -	spin_lock(&cdrom_lock);
> +	mutex_lock(&cdrom_mutex);
>  	cdi = topCdromPtr;
>  	while (cdi && cdi != unreg) {
>  		prev = cdi;
> @@ -460,7 +460,7 @@ int unregister_cdrom(struct cdrom_device_info *unreg)
>  	}
>  
>  	if (cdi == NULL) {
> -		spin_unlock(&cdrom_lock);
> +		mutex_unlock(&cdrom_mutex);
>  		return -2;
>  	}
>  	if (prev)
> @@ -468,7 +468,7 @@ int unregister_cdrom(struct cdrom_device_info *unreg)
>  	else
>  		topCdromPtr = cdi->next;
>  
> -	spin_unlock(&cdrom_lock);
> +	mutex_unlock(&cdrom_mutex);
>  
>  	if (cdi->exit)
>  		cdi->exit(cdi);
> @@ -3289,103 +3289,112 @@ static struct cdrom_sysctl_settings {
>  	int	check;			/* check media type */
>  } cdrom_sysctl_settings;
>  
> +static int cdrom_print_int(const char *header, int val, char *info, int *pos)
> +{
The val is diffrent for every devices, isn't it?
> +	const int max_size = sizeof(cdrom_sysctl_settings.info);
> +	struct cdrom_device_info *cdi;
> +	int ret;
> +
> +	ret = scnprintf(info + *pos, max_size, header);
> +	if (!ret)
> +		return 1;
> +
> +	*pos += ret;
> +
> +	for (cdi = topCdromPtr; cdi; cdi = cdi->next) {
> +		ret = scnprintf(info + *pos, max_size, "\t%d", val);
The max_size should be max_size - *pos
> +		if (!ret)
> +			return 1;
> +		*pos += ret;
> +	}
> +
> +	return 0;
> +}
> +
>  static int cdrom_sysctl_info(ctl_table *ctl, int write, struct file * filp,
>                             void __user *buffer, size_t *lenp, loff_t *ppos)
>  {
> -        int pos;
> +	int pos;
>  	struct cdrom_device_info *cdi;
>  	char *info = cdrom_sysctl_settings.info;
> +	const int max_size = sizeof(cdrom_sysctl_settings.info);
>  	
>  	if (!*lenp || (*ppos && !write)) {
>  		*lenp = 0;
>  		return 0;
>  	}
>  
> +	mutex_lock(&cdrom_mutex);
> +
>  	pos = sprintf(info, "CD-ROM information, " VERSION "\n");
>  	
> -	pos += sprintf(info+pos, "\ndrive name:\t");
> -	for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
> -	    pos += sprintf(info+pos, "\t%s", cdi->name);
> -
> -	pos += sprintf(info+pos, "\ndrive speed:\t");
> -	for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
> -	    pos += sprintf(info+pos, "\t%d", cdi->speed);
> -
> -	pos += sprintf(info+pos, "\ndrive # of slots:");
> -	for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
> -	    pos += sprintf(info+pos, "\t%d", cdi->capacity);
> -
> -	pos += sprintf(info+pos, "\nCan close tray:\t");
> -	for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
> -	    pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_CLOSE_TRAY) != 0);
> -
> -	pos += sprintf(info+pos, "\nCan open tray:\t");
> -	for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
> -	    pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_OPEN_TRAY) != 0);
> -
> -	pos += sprintf(info+pos, "\nCan lock tray:\t");
> -	for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
> -	    pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_LOCK) != 0);
> -
> -	pos += sprintf(info+pos, "\nCan change speed:");
> -	for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
> -	    pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_SELECT_SPEED) != 0);
> -
> -	pos += sprintf(info+pos, "\nCan select disk:");
> -	for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
> -	    pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_SELECT_DISC) != 0);
> -
> -	pos += sprintf(info+pos, "\nCan read multisession:");
> -	for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
> -	    pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_MULTI_SESSION) != 0);
> -
> -	pos += sprintf(info+pos, "\nCan read MCN:\t");
> -	for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
> -	    pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_MCN) != 0);
> -
> -	pos += sprintf(info+pos, "\nReports media changed:");
> -	for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
> -	    pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_MEDIA_CHANGED) != 0);
> -
> -	pos += sprintf(info+pos, "\nCan play audio:\t");
> -	for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
> -	    pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_PLAY_AUDIO) != 0);
> -
> -	pos += sprintf(info+pos, "\nCan write CD-R:\t");
> -	for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
> -	    pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_CD_R) != 0);
> -
> -	pos += sprintf(info+pos, "\nCan write CD-RW:");
> -	for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
> -	    pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_CD_RW) != 0);
> -
> -	pos += sprintf(info+pos, "\nCan read DVD:\t");
> -	for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
> -	    pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_DVD) != 0);
> -
> -	pos += sprintf(info+pos, "\nCan write DVD-R:");
> -	for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
> -	    pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_DVD_R) != 0);
> -
> -	pos += sprintf(info+pos, "\nCan write DVD-RAM:");
> -	for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
> -	    pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_DVD_RAM) != 0);
> -
> -	pos += sprintf(info+pos, "\nCan read MRW:\t");
> -	for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
> -	    pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_MRW) != 0);
> -
> -	pos += sprintf(info+pos, "\nCan write MRW:\t");
> -	for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
> -	    pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_MRW_W) != 0);
> -
> -	pos += sprintf(info+pos, "\nCan write RAM:\t");
> -	for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
> -	    pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_RAM) != 0);
> -
> -	strcpy(info+pos,"\n\n");
> -		
> -        return proc_dostring(ctl, write, filp, buffer, lenp, ppos);
> +	pos += scnprintf(info + pos, max_size - pos, "\ndrive name:\t");
> +	for (cdi = topCdromPtr; cdi != NULL; cdi = cdi->next)
> +		pos += scnprintf(info + pos, max_size - pos, "\t%s", cdi->name);
> +
> +	if (cdrom_print_int("\ndrive speed:\t", cdi->speed, info, &pos))
Now the cdi is NULL
> +		goto done;
> +	if (cdrom_print_int("\ndrive # of slots:", cdi->capacity, info, &pos))
> +		goto done;
> +	if (cdrom_print_int("\nCan close tray:\t",
> +				CDROM_CAN(CDC_CLOSE_TRAY) != 0, info, &pos))
> +		goto done;
> +	if (cdrom_print_int("\nCan open tray:\t",
> +				CDROM_CAN(CDC_OPEN_TRAY) != 0, info, &pos))
> +		goto done;
> +	if (cdrom_print_int("\nCan lock tray:\t",
> +				CDROM_CAN(CDC_LOCK) != 0, info, &pos))
> +		goto done;
> +	if (cdrom_print_int("\nCan change speed:\t",
> +				CDROM_CAN(CDC_SELECT_SPEED) != 0, info, &pos))
> +		goto done;
> +	if (cdrom_print_int("\nCan select disk:\t",
> +				CDROM_CAN(CDC_SELECT_DISC) != 0, info, &pos))
> +		goto done;
> +	if (cdrom_print_int("\nCan read multisession:",
> +				CDROM_CAN(CDC_MULTI_SESSION) != 0, info, &pos))
> +		goto done;
> +	if (cdrom_print_int("\nCan read MCN:",
> +				CDROM_CAN(CDC_MCN) != 0, info, &pos))
> +		goto done;
> +	if (cdrom_print_int("\nReports media changed:",
> +				CDROM_CAN(CDC_MEDIA_CHANGED) != 0, info, &pos))
> +		goto done;
> +	if (cdrom_print_int("\nCan play audio:",
> +				CDROM_CAN(CDC_PLAY_AUDIO) != 0, info, &pos))
> +		goto done;
> +	if (cdrom_print_int("\nCan write CD-R:\t",
> +				CDROM_CAN(CDC_CD_R) != 0, info, &pos))
> +		goto done;
> +	if (cdrom_print_int("\nCan write CD-RW:\t",
> +				CDROM_CAN(CDC_CD_RW) != 0, info, &pos))
> +		goto done;
> +	if (cdrom_print_int("\nCan read DVD:\t",
> +				CDROM_CAN(CDC_DVD) != 0, info, &pos))
> +		goto done;
> +	if (cdrom_print_int("\nCan write DVD-R:\t",
> +				CDROM_CAN(CDC_DVD_R) != 0, info, &pos))
> +		goto done;
> +	if (cdrom_print_int("\nCan write DVD-RAM:\t",
> +				CDROM_CAN(CDC_DVD_RAM) != 0, info, &pos))
> +		goto done;
> +	if (cdrom_print_int("\nCan read MRW:\t",
> +				CDROM_CAN(CDC_MRW) != 0, info, &pos))
> +		goto done;
> +	if (cdrom_print_int("\nCan write MRW:\t",
> +				CDROM_CAN(CDC_MRW_W) != 0, info, &pos))
> +		goto done;
> +	if (cdrom_print_int("\nCan write RAM:\t",
> +				CDROM_CAN(CDC_RAM) != 0, info, &pos))
> +		goto done;
> +	if (!scnprintf(info + pos, max_size - pos, "\n\n"))
> +		goto done;
> +doit:
> +	mutex_unlock(&cdrom_mutex);
> +	return proc_dostring(ctl, write, filp, buffer, lenp, ppos);
> +done:
> +	printk(KERN_INFO "cdrom: info buffer too small\n");
> +	goto doit;
>  }
>  
>  /* Unfortunately, per device settings are not implemented through
> 
> -- 
> Jens Axboe
And there's several more '\t' in the printed strings. so the result not aligned right.

I rewite the patch according to yours, please help to check. Pass the cdrom_print_info function one more argument for diffrent type printing, do you have better solutions for this? thanks for replying :)


Signed-off-by: Dave Young <hidave.darkstar@gmail.com>
---
diff -upr linux/drivers/cdrom/cdrom.c linux.new/drivers/cdrom/cdrom.c
--- linux/drivers/cdrom/cdrom.c	2007-06-18 12:36:40.000000000 +0000
+++ linux.new/drivers/cdrom/cdrom.c	2007-06-18 12:38:51.000000000 +0000
@@ -302,7 +302,7 @@ module_param(lockdoor, bool, 0);
 module_param(check_media_type, bool, 0);
 module_param(mrw_format_restart, bool, 0);
 
-static DEFINE_SPINLOCK(cdrom_lock);
+static DEFINE_MUTEX(cdrom_mutex);
 
 static const char *mrw_format_status[] = {
 	"not mrw",
@@ -438,10 +438,10 @@ int register_cdrom(struct cdrom_device_i
 		cdo->generic_packet = cdrom_dummy_generic_packet;
 
 	cdinfo(CD_REG_UNREG, "drive \"/dev/%s\" registered\n", cdi->name);
-	spin_lock(&cdrom_lock);
+	mutex_lock(&cdrom_mutex);
 	cdi->next = topCdromPtr; 	
 	topCdromPtr = cdi;
-	spin_unlock(&cdrom_lock);
+	mutex_unlock(&cdrom_mutex);
 	return 0;
 }
 #undef ENSURE
@@ -452,7 +452,7 @@ int unregister_cdrom(struct cdrom_device
 	cdinfo(CD_OPEN, "entering unregister_cdrom\n"); 
 
 	prev = NULL;
-	spin_lock(&cdrom_lock);
+	mutex_lock(&cdrom_mutex);
 	cdi = topCdromPtr;
 	while (cdi && cdi != unreg) {
 		prev = cdi;
@@ -460,7 +460,7 @@ int unregister_cdrom(struct cdrom_device
 	}
 
 	if (cdi == NULL) {
-		spin_unlock(&cdrom_lock);
+		mutex_unlock(&cdrom_mutex);
 		return -2;
 	}
 	if (prev)
@@ -468,7 +468,7 @@ int unregister_cdrom(struct cdrom_device
 	else
 		topCdromPtr = cdi->next;
 
-	spin_unlock(&cdrom_lock);
+	mutex_unlock(&cdrom_mutex);
 
 	if (cdi->exit)
 		cdi->exit(cdi);
@@ -3289,103 +3289,137 @@ static struct cdrom_sysctl_settings {
 	int	check;			/* check media type */
 } cdrom_sysctl_settings;
 
+enum cdrom_print_option {
+	CTL_NAME,
+	CTL_SPEED,
+	CTL_SLOTS,
+	CTL_CAPABILITY
+};
+
+static int cdrom_print_info(const char *header, int val, char *info,
+				int *pos, enum cdrom_print_option option)
+{
+	const int max_size = sizeof(cdrom_sysctl_settings.info);
+	struct cdrom_device_info *cdi;
+	int ret;
+
+	ret = scnprintf(info + *pos, max_size - *pos, header);
+	if (!ret)
+		return 1;
+
+	*pos += ret;
+
+	for (cdi = topCdromPtr; cdi; cdi = cdi->next) {
+		switch (option) {
+		case CTL_NAME:
+			ret = scnprintf(info + *pos, max_size - *pos,
+					"\t%s", cdi->name);
+			break;
+		case CTL_SPEED:
+			ret = scnprintf(info + *pos, max_size - *pos,
+					"\t%d", cdi->speed);
+			break;
+		case CTL_SLOTS:
+			ret = scnprintf(info + *pos, max_size - *pos,
+					"\t%d", cdi->capacity);
+			break;
+		case CTL_CAPABILITY:
+			ret = scnprintf(info + *pos, max_size - *pos,
+					"\t%d", CDROM_CAN(val) != 0);
+			break;
+		default:
+			printk(KERN_INFO "cdrom: invalid option%d\n", option);
+			return 1;
+		}
+		if (!ret)
+			return 1;
+		*pos += ret;
+	}
+
+	return 0;
+}
+
 static int cdrom_sysctl_info(ctl_table *ctl, int write, struct file * filp,
                            void __user *buffer, size_t *lenp, loff_t *ppos)
 {
-        int pos;
-	struct cdrom_device_info *cdi;
+	int pos;
 	char *info = cdrom_sysctl_settings.info;
+	const int max_size = sizeof(cdrom_sysctl_settings.info);
 	
 	if (!*lenp || (*ppos && !write)) {
 		*lenp = 0;
 		return 0;
 	}
 
+	mutex_lock(&cdrom_mutex);
+
 	pos = sprintf(info, "CD-ROM information, " VERSION "\n");
 	
-	pos += sprintf(info+pos, "\ndrive name:\t");
-	for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
-	    pos += sprintf(info+pos, "\t%s", cdi->name);
-
-	pos += sprintf(info+pos, "\ndrive speed:\t");
-	for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
-	    pos += sprintf(info+pos, "\t%d", cdi->speed);
-
-	pos += sprintf(info+pos, "\ndrive # of slots:");
-	for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
-	    pos += sprintf(info+pos, "\t%d", cdi->capacity);
-
-	pos += sprintf(info+pos, "\nCan close tray:\t");
-	for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
-	    pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_CLOSE_TRAY) != 0);
-
-	pos += sprintf(info+pos, "\nCan open tray:\t");
-	for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
-	    pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_OPEN_TRAY) != 0);
-
-	pos += sprintf(info+pos, "\nCan lock tray:\t");
-	for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
-	    pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_LOCK) != 0);
-
-	pos += sprintf(info+pos, "\nCan change speed:");
-	for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
-	    pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_SELECT_SPEED) != 0);
-
-	pos += sprintf(info+pos, "\nCan select disk:");
-	for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
-	    pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_SELECT_DISC) != 0);
-
-	pos += sprintf(info+pos, "\nCan read multisession:");
-	for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
-	    pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_MULTI_SESSION) != 0);
-
-	pos += sprintf(info+pos, "\nCan read MCN:\t");
-	for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
-	    pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_MCN) != 0);
-
-	pos += sprintf(info+pos, "\nReports media changed:");
-	for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
-	    pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_MEDIA_CHANGED) != 0);
-
-	pos += sprintf(info+pos, "\nCan play audio:\t");
-	for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
-	    pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_PLAY_AUDIO) != 0);
-
-	pos += sprintf(info+pos, "\nCan write CD-R:\t");
-	for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
-	    pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_CD_R) != 0);
-
-	pos += sprintf(info+pos, "\nCan write CD-RW:");
-	for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
-	    pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_CD_RW) != 0);
-
-	pos += sprintf(info+pos, "\nCan read DVD:\t");
-	for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
-	    pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_DVD) != 0);
-
-	pos += sprintf(info+pos, "\nCan write DVD-R:");
-	for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
-	    pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_DVD_R) != 0);
-
-	pos += sprintf(info+pos, "\nCan write DVD-RAM:");
-	for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
-	    pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_DVD_RAM) != 0);
-
-	pos += sprintf(info+pos, "\nCan read MRW:\t");
-	for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
-	    pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_MRW) != 0);
-
-	pos += sprintf(info+pos, "\nCan write MRW:\t");
-	for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
-	    pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_MRW_W) != 0);
-
-	pos += sprintf(info+pos, "\nCan write RAM:\t");
-	for (cdi=topCdromPtr;cdi!=NULL;cdi=cdi->next)
-	    pos += sprintf(info+pos, "\t%d", CDROM_CAN(CDC_RAM) != 0);
-
-	strcpy(info+pos,"\n\n");
-		
-        return proc_dostring(ctl, write, filp, buffer, lenp, ppos);
+	if (cdrom_print_info("\ndrive name:\t", 0, info, &pos, CTL_NAME))
+		goto done;
+	if (cdrom_print_info("\ndrive speed:\t", 0, info, &pos, CTL_SPEED))
+		goto done;
+	if (cdrom_print_info("\ndrive # of slots:", 0, info, &pos, CTL_SLOTS))
+		goto done;
+	if (cdrom_print_info("\nCan close tray:\t",
+				CDC_CLOSE_TRAY, info, &pos, CTL_CAPABILITY))
+		goto done;
+	if (cdrom_print_info("\nCan open tray:\t",
+				CDC_OPEN_TRAY, info, &pos, CTL_CAPABILITY))
+		goto done;
+	if (cdrom_print_info("\nCan lock tray:\t",
+				CDC_LOCK, info, &pos, CTL_CAPABILITY))
+		goto done;
+	if (cdrom_print_info("\nCan change speed:",
+				CDC_SELECT_SPEED, info, &pos, CTL_CAPABILITY))
+		goto done;
+	if (cdrom_print_info("\nCan select disk:",
+				CDC_SELECT_DISC, info, &pos, CTL_CAPABILITY))
+		goto done;
+	if (cdrom_print_info("\nCan read multisession:",
+				CDC_MULTI_SESSION, info, &pos, CTL_CAPABILITY))
+		goto done;
+	if (cdrom_print_info("\nCan read MCN:\t",
+				CDC_MCN, info, &pos, CTL_CAPABILITY))
+		goto done;
+	if (cdrom_print_info("\nReports media changed:",
+				CDC_MEDIA_CHANGED, info, &pos, CTL_CAPABILITY))
+		goto done;
+	if (cdrom_print_info("\nCan play audio:\t",
+				CDC_PLAY_AUDIO, info, &pos, CTL_CAPABILITY))
+		goto done;
+	if (cdrom_print_info("\nCan write CD-R:\t",
+				CDC_CD_R, info, &pos, CTL_CAPABILITY))
+		goto done;
+	if (cdrom_print_info("\nCan write CD-RW:",
+				CDC_CD_RW, info, &pos, CTL_CAPABILITY))
+		goto done;
+	if (cdrom_print_info("\nCan read DVD:\t",
+				CDC_DVD, info, &pos, CTL_CAPABILITY))
+		goto done;
+	if (cdrom_print_info("\nCan write DVD-R:",
+				CDC_DVD_R, info, &pos, CTL_CAPABILITY))
+		goto done;
+	if (cdrom_print_info("\nCan write DVD-RAM:",
+				CDC_DVD_RAM, info, &pos, CTL_CAPABILITY))
+		goto done;
+	if (cdrom_print_info("\nCan read MRW:\t",
+				CDC_MRW, info, &pos, CTL_CAPABILITY))
+		goto done;
+	if (cdrom_print_info("\nCan write MRW:\t",
+				CDC_MRW_W, info, &pos, CTL_CAPABILITY))
+		goto done;
+	if (cdrom_print_info("\nCan write RAM:\t",
+				CDC_RAM, info, &pos, CTL_CAPABILITY))
+		goto done;
+	if (!scnprintf(info + pos, max_size - pos, "\n\n"))
+		goto done;
+doit:
+	mutex_unlock(&cdrom_mutex);
+	return proc_dostring(ctl, write, filp, buffer, lenp, ppos);
+done:
+	printk(KERN_INFO "cdrom: info buffer too small\n");
+	goto doit;
 }
 
 /* Unfortunately, per device settings are not implemented through
Only in linux/drivers/cdrom: cdrom.old


--
Regards
dave

Re: [PATCH] cdrom_sysctl_info fix

[Jens Axboe]

On Mon, Jun 18 2007, Dave Young wrote:
> I rewite the patch according to yours, please help to check. Pass the
> cdrom_print_info function one more argument for diffrent type
> printing, do you have better solutions for this? thanks for replying
> :)

Yep that looks much better! I trust you booted and tested this and the
output looks correct?

-- 
Jens Axboe

Re: [PATCH] cdrom_sysctl_info fix

[dave young]

Hi,
> Yep that looks much better! I trust you booted and tested this and the
> output looks correct?
Yes, I tested and the output is correct.

BTW, another problem, I can't find the CONFIG_CDROM option in kernel
config menu. Is it deperacated? If it is true, is the module part
still necessary?

Regards
dave

Re: [PATCH] cdrom_sysctl_info fix

[Jens Axboe]

On Mon, Jun 18 2007, dave young wrote:
> Hi,
> >Yep that looks much better! I trust you booted and tested this and the
> >output looks correct?
> Yes, I tested and the output is correct.

Good

> BTW, another problem, I can't find the CONFIG_CDROM option in kernel
> config menu. Is it deperacated? If it is true, is the module part
> still necessary?

cdrom.o is a hardware independent helper module, it gets included if you
use atapi/scsi/etc cdrom drivers. See drivers/cdrom/Makefile.

-- 
Jens Axboe

Re: [PATCH] cdrom_sysctl_info fix

[dave young]

Hi,
> > BTW, another problem, I can't find the CONFIG_CDROM option in kernel
> > config menu. Is it deperacated? If it is true, is the module part
> > still necessary?
>
> cdrom.o is a hardware independent helper module, it gets included if you
> use atapi/scsi/etc cdrom drivers. See drivers/cdrom/Makefile.
>
I know. Sorry for my bad english, I means MODULE's part maybe
deperacated, not uniform cdrom driver itsself.

For example  the module_init, module_exit and MODULE_LICENSE, I
remember it could be a standalone module in 2.4 kernels. But now seems
not possible. am I right?

Regards
dave

Re: [PATCH] cdrom_sysctl_info fix

[Jens Axboe]

On Mon, Jun 18 2007, dave young wrote:
> Hi,
> >> BTW, another problem, I can't find the CONFIG_CDROM option in kernel
> >> config menu. Is it deperacated? If it is true, is the module part
> >> still necessary?
> >
> >cdrom.o is a hardware independent helper module, it gets included if you
> >use atapi/scsi/etc cdrom drivers. See drivers/cdrom/Makefile.
> >
> I know. Sorry for my bad english, I means MODULE's part maybe
> deperacated, not uniform cdrom driver itsself.
> 
> For example  the module_init, module_exit and MODULE_LICENSE, I
> remember it could be a standalone module in 2.4 kernels. But now seems
> not possible. am I right?

It is a stand-alone module.

-- 
Jens Axboe

Questions on one PowerPC assembly instruction from hash_page

[gshan]

Hey Guys,

I can't understand the following instructions from 
arch/ppc/mm/hashtable.S::hash_page. If I got the right design, the 
following instruction is to get the PMD (Page Middle Descritor) because 
Linux for 32-bits PowerPC cut page table into 3 domains: root, PMD, PTE. 
The top bits (22 to 31 bit) is the index for PMD, and the next 10 bits 
(12 to 21 bit) is the index for PTE in the associative PMD. The 
remaining 12 bits (0 to 11 bit) indicated the page size (4KB). However, 
the following instruction polled [8-17] bits instead of [22-31] bits as 
expected. Anybody could give me answer?

r4 is the address that caused the DSI
r5 is the address of swapper_pg_dir if we are under kernel mode.

rlwimi r5,r4,12,20,29 /* insert top 10 bits of address */

Thanks,
Gavin