clang static analysis reports this representative issue
pcd.c:832:22: warning: Assigned value is garbage
or undefined
tochdr->cdth_trk0 = buffer[2];
^ ~~~~~~~~~
If the call to pcd_atapi fails, buffer is an unknown
state. Passing an unknown buffer back to the user
can leak information and is a security risk.
Check before returning this buffer to the user.
The per-case variables cmd and buffer are common.
Change their scope to function level.
Change colliding parameter name cmd to request.
Cleanup whitespace
pcd.c comment
/* the audio_ioctl stuff is adapted from sr_ioctl.c */
Shows there is a similar problem in sr_ioctl.c
sr_ioctl.c uses this pattern
result = sr_do_ioctl(cd, &cgc);
to-user = buffer[];
kfree(buffer);
return result;
Check result and jump over the use of buffer
if there is an error.
result = sr_do_ioctl(cd, &cgc);
if (result)
goto err;
to-user = buffer[];
err:
kfree(buffer);
return result;
Additionally initialize the buffer to zero.
This problem can be seen in the 2.4.0 kernel
However this scm only goes back as far as 2.6.12
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Tom Rix <trix@xxxxxxxxxx>
---
drivers/block/paride/pcd.c | 87 +++++++++++++++++---------------------
drivers/scsi/sr_ioctl.c | 15 +++++--
2 files changed, 50 insertions(+), 52 deletions(-)
diff --git a/drivers/block/paride/pcd.c b/drivers/block/paride/pcd.c
index f462ad67931a..2315918e3647 100644
--- a/drivers/block/paride/pcd.c
+++ b/drivers/block/paride/pcd.c
@@ -810,67 +810,56 @@ static void do_pcd_read_drq(void)
/* the audio_ioctl stuff is adapted from sr_ioctl.c */
-static int pcd_audio_ioctl(struct cdrom_device_info *cdi, unsigned int cmd, void *arg)
+static int pcd_audio_ioctl(struct cdrom_device_info *cdi, unsigned int request, void *arg)
{
struct pcd_unit *cd = cdi->handle;
+ char cmd[12] = { GPCMD_READ_TOC_PMA_ATIP, 0, 0, 0, 0, 0, 0, 0, 12, 0, 0, 0 };
+ char buffer[32] = {};
- switch (cmd) {
-
+ switch (request) {
case CDROMREADTOCHDR:
+ {
+ struct cdrom_tochdr *tochdr =
+ (struct cdrom_tochdr *) arg;
- {
- char cmd[12] =
- { GPCMD_READ_TOC_PMA_ATIP, 0, 0, 0, 0, 0, 0, 0, 12,
- 0, 0, 0 };
- struct cdrom_tochdr *tochdr =
- (struct cdrom_tochdr *) arg;
- char buffer[32];
- int r;
-
- r = pcd_atapi(cd, cmd, 12, buffer, "read toc header");
+ if (pcd_atapi(cd, cmd, 12, buffer, "read toc header"))
+ return -EIO;
- tochdr->cdth_trk0 = buffer[2];
- tochdr->cdth_trk1 = buffer[3];
+ tochdr->cdth_trk0 = buffer[2];
+ tochdr->cdth_trk1 = buffer[3];
- return r ? -EIO : 0;
- }
+ return 0;
+ }
case CDROMREADTOCENTRY:
-
- {
- char cmd[12] =
- { GPCMD_READ_TOC_PMA_ATIP, 0, 0, 0, 0, 0, 0, 0, 12,
- 0, 0, 0 };
-
- struct cdrom_tocentry *tocentry =
- (struct cdrom_tocentry *) arg;
- unsigned char buffer[32];
- int r;
-
- cmd[1] =
- (tocentry->cdte_format == CDROM_MSF ? 0x02 : 0);
- cmd[6] = tocentry->cdte_track;
-
- r = pcd_atapi(cd, cmd, 12, buffer, "read toc entry");
-
- tocentry->cdte_ctrl = buffer[5] & 0xf;
- tocentry->cdte_adr = buffer[5] >> 4;
- tocentry->cdte_datamode =
- (tocentry->cdte_ctrl & 0x04) ? 1 : 0;
- if (tocentry->cdte_format == CDROM_MSF) {
- tocentry->cdte_addr.msf.minute = buffer[9];
- tocentry->cdte_addr.msf.second = buffer[10];
- tocentry->cdte_addr.msf.frame = buffer[11];
- } else
- tocentry->cdte_addr.lba =
- (((((buffer[8] << 8) + buffer[9]) << 8)
- + buffer[10]) << 8) + buffer[11];
-
- return r ? -EIO : 0;
+ {
+ struct cdrom_tocentry *tocentry =
+ (struct cdrom_tocentry *) arg;
+
+ cmd[1] = (tocentry->cdte_format == CDROM_MSF ? 0x02 : 0);
+ cmd[6] = tocentry->cdte_track;
+
+ if (pcd_atapi(cd, cmd, 12, buffer, "read toc entry"))
+ return -EIO;
+
+ tocentry->cdte_ctrl = buffer[5] & 0xf;
+ tocentry->cdte_adr = buffer[5] >> 4;
+ tocentry->cdte_datamode =
+ (tocentry->cdte_ctrl & 0x04) ? 1 : 0;
+ if (tocentry->cdte_format == CDROM_MSF) {
+ tocentry->cdte_addr.msf.minute = buffer[9];
+ tocentry->cdte_addr.msf.second = buffer[10];
+ tocentry->cdte_addr.msf.frame = buffer[11];
+ } else {
+ tocentry->cdte_addr.lba =
+ (((((buffer[8] << 8) + buffer[9]) << 8)
+ + buffer[10]) << 8) + buffer[11];
}
- default:
+ return 0;
+ }
+ default:
return -ENOSYS;
}
}
diff --git a/drivers/scsi/sr_ioctl.c b/drivers/scsi/sr_ioctl.c
index ddd00efc4882..fbdb5124d7f7 100644
--- a/drivers/scsi/sr_ioctl.c
+++ b/drivers/scsi/sr_ioctl.c
@@ -41,7 +41,7 @@ static int sr_read_tochdr(struct cdrom_device_info *cdi,
int result;
unsigned char *buffer;
- buffer = kmalloc(32, GFP_KERNEL);
+ buffer = kzalloc(32, GFP_KERNEL);
if (!buffer)
return -ENOMEM;
@@ -55,10 +55,13 @@ static int sr_read_tochdr(struct cdrom_device_info *cdi,
cgc.data_direction = DMA_FROM_DEVICE;
result = sr_do_ioctl(cd, &cgc);
+ if (result)
+ goto err;
tochdr->cdth_trk0 = buffer[2];
tochdr->cdth_trk1 = buffer[3];
+err:
kfree(buffer);
return result;
}
@@ -71,7 +74,7 @@ static int sr_read_tocentry(struct cdrom_device_info *cdi,
int result;
unsigned char *buffer;
- buffer = kmalloc(32, GFP_KERNEL);
+ buffer = kzalloc(32, GFP_KERNEL);
if (!buffer)
return -ENOMEM;
@@ -86,6 +89,8 @@ static int sr_read_tocentry(struct cdrom_device_info *cdi,
cgc.data_direction = DMA_FROM_DEVICE;
result = sr_do_ioctl(cd, &cgc);
+ if (result)
+ goto err;
tocentry->cdte_ctrl = buffer[5] & 0xf;
tocentry->cdte_adr = buffer[5] >> 4;
@@ -98,6 +103,7 @@ static int sr_read_tocentry(struct cdrom_device_info *cdi,
tocentry->cdte_addr.lba = (((((buffer[8] << 8) + buffer[9]) << 8)
+ buffer[10]) << 8) + buffer[11];
+err:
kfree(buffer);
return result;
}
@@ -384,7 +390,7 @@ int sr_get_mcn(struct cdrom_device_info *cdi, struct cdrom_mcn *mcn)
{
Scsi_CD *cd = cdi->handle;
struct packet_command cgc;
- char *buffer = kmalloc(32, GFP_KERNEL);
+ char *buffer = kzalloc(32, GFP_KERNEL);
int result;
if (!buffer)
@@ -400,10 +406,13 @@ int sr_get_mcn(struct cdrom_device_info *cdi, struct cdrom_mcn *mcn)
cgc.data_direction = DMA_FROM_DEVICE;
cgc.timeout = IOCTL_TIMEOUT;
result = sr_do_ioctl(cd, &cgc);
+ if (result)
+ goto err;
memcpy(mcn->medium_catalog_number, buffer + 9, 13);
mcn->medium_catalog_number[13] = 0;
+err:
kfree(buffer);
return result;
}
--
2.27.0
