[PATCH] scsi: esas2r: Fix format string type mistakes
From: Emese Revfy
This adds the missing __printf attribute which allows compile time
format string checking (and will be used by the coming initify gcc
plugin). Additionally, this fixes the warnings exposed by the attribute.
Signed-off-by: Emese Revfy
[kees: split scsi/acpi, merged attr and fix, new commit messages]
Signed-off-by: Kees Cook
---
drivers/scsi/esas2r/esas2r_init.c | 2 +-
drivers/scsi/esas2r/esas2r_ioctl.c | 2 +-
drivers/scsi/esas2r/esas2r_log.h | 4 ++--
drivers/scsi/esas2r/esas2r_main.c | 4 ++--
4 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/scsi/esas2r/esas2r_init.c
b/drivers/scsi/esas2r/esas2r_init.c
index d6e53aee2295..6432a50b26d8 100644
--- a/drivers/scsi/esas2r/esas2r_init.c
+++ b/drivers/scsi/esas2r/esas2r_init.c
@@ -237,7 +237,7 @@ static void esas2r_claim_interrupts(struct esas2r_adapter
*a)
flags |= IRQF_SHARED;
esas2r_log(ESAS2R_LOG_INFO,
- "esas2r_claim_interrupts irq=%d (%p, %s, %x)",
+ "esas2r_claim_interrupts irq=%d (%p, %s, %lx)",
a->pcid->irq, a, a->name, flags);
if (request_irq(a->pcid->irq,
diff --git a/drivers/scsi/esas2r/esas2r_ioctl.c
b/drivers/scsi/esas2r/esas2r_ioctl.c
index 3e8483410f61..34976f9a1a10 100644
--- a/drivers/scsi/esas2r/esas2r_ioctl.c
+++ b/drivers/scsi/esas2r/esas2r_ioctl.c
@@ -1301,7 +1301,7 @@ int esas2r_ioctl_handler(void *hostdata, int cmd, void
__user *arg)
ioctl = kzalloc(sizeof(struct atto_express_ioctl), GFP_KERNEL);
if (ioctl == NULL) {
esas2r_log(ESAS2R_LOG_WARN,
- "ioctl_handler kzalloc failed for %d bytes",
+ "ioctl_handler kzalloc failed for %lu bytes",
sizeof(struct atto_express_ioctl));
return -ENOMEM;
}
diff --git a/drivers/scsi/esas2r/esas2r_log.h b/drivers/scsi/esas2r/esas2r_log.h
index 7b6397bb5b94..75b9d23cd736 100644
--- a/drivers/scsi/esas2r/esas2r_log.h
+++ b/drivers/scsi/esas2r/esas2r_log.h
@@ -61,8 +61,8 @@ enum {
#endif
};
-int esas2r_log(const long level, const char *format, ...);
-int esas2r_log_dev(const long level,
+__printf(2, 3) int esas2r_log(const long level, const char *format, ...);
+__printf(3, 4) int esas2r_log_dev(const long level,
const struct device *dev,
const char *format,
...);
diff --git a/drivers/scsi/esas2r/esas2r_main.c
b/drivers/scsi/esas2r/esas2r_main.c
index 5092c821d088..072d07b45043 100644
--- a/drivers/scsi/esas2r/esas2r_main.c
+++ b/drivers/scsi/esas2r/esas2r_main.c
@@ -198,7 +198,7 @@ static ssize_t write_hw(struct file *file, struct kobject
*kobj,
GFP_KERNEL);
if (a->local_atto_ioctl == NULL) {
esas2r_log(ESAS2R_LOG_WARN,
- "write_hw kzalloc failed for %d bytes",
+ "write_hw kzalloc failed for %lu bytes",
sizeof(struct atto_ioctl));
return -ENOMEM;
}
@@ -1186,7 +1186,7 @@ static int esas2r_dev_targ_reset(struct scsi_cmnd *cmd,
bool target_reset)
} else {
esas2r_log(ESAS2R_LOG_CRIT,
"unable to allocate a request for a "
- "device reset (%d:%d)!",
+ "device reset (%d:%llu)!",
cmd->device->id,
cmd->device->lun);
}
--
2.7.4
--
Kees Cook
Nexus Security
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] scsi/bfa: use designated initializers
Prepare to mark sensitive kernel structures for randomization by making
sure they're using designated initializers. These were identified during
allyesconfig builds of x86, arm, and arm64, with most initializer fixes
extracted from grsecurity.
Signed-off-by: Kees Cook
---
drivers/scsi/bfa/bfa_fcs.c | 15 +++
drivers/scsi/bfa/bfa_fcs_lport.c | 29 -
drivers/scsi/bfa/bfa_modules.h | 12 ++--
3 files changed, 37 insertions(+), 19 deletions(-)
diff --git a/drivers/scsi/bfa/bfa_fcs.c b/drivers/scsi/bfa/bfa_fcs.c
index 1e7e139d71ea..f8f76501af90 100644
--- a/drivers/scsi/bfa/bfa_fcs.c
+++ b/drivers/scsi/bfa/bfa_fcs.c
@@ -39,10 +39,17 @@ struct bfa_fcs_mod_s {
#define BFA_FCS_MODULE(_mod) { _mod ## _modinit, _mod ## _modexit }
static struct bfa_fcs_mod_s fcs_modules[] = {
- { bfa_fcs_port_attach, NULL, NULL },
- { bfa_fcs_uf_attach, NULL, NULL },
- { bfa_fcs_fabric_attach, bfa_fcs_fabric_modinit,
- bfa_fcs_fabric_modexit },
+ {
+ .attach = bfa_fcs_port_attach,
+ },
+ {
+ .attach = bfa_fcs_uf_attach,
+ },
+ {
+ .attach = bfa_fcs_fabric_attach,
+ .modinit = bfa_fcs_fabric_modinit,
+ .modexit = bfa_fcs_fabric_modexit
+ },
};
/*
diff --git a/drivers/scsi/bfa/bfa_fcs_lport.c b/drivers/scsi/bfa/bfa_fcs_lport.c
index 4ddda72f60e6..64860c730ec9 100644
--- a/drivers/scsi/bfa/bfa_fcs_lport.c
+++ b/drivers/scsi/bfa/bfa_fcs_lport.c
@@ -90,15 +90,26 @@ static struct {
void(*offline) (struct bfa_fcs_lport_s *port);
} __port_action[] = {
{
- bfa_fcs_lport_unknown_init, bfa_fcs_lport_unknown_online,
- bfa_fcs_lport_unknown_offline}, {
- bfa_fcs_lport_fab_init, bfa_fcs_lport_fab_online,
- bfa_fcs_lport_fab_offline}, {
- bfa_fcs_lport_n2n_init, bfa_fcs_lport_n2n_online,
- bfa_fcs_lport_n2n_offline}, {
- bfa_fcs_lport_loop_init, bfa_fcs_lport_loop_online,
- bfa_fcs_lport_loop_offline},
- };
+ .init = bfa_fcs_lport_unknown_init,
+ .online = bfa_fcs_lport_unknown_online,
+ .offline = bfa_fcs_lport_unknown_offline
+ },
+ {
+ .init = bfa_fcs_lport_fab_init,
+ .online = bfa_fcs_lport_fab_online,
+ .offline = bfa_fcs_lport_fab_offline
+ },
+ {
+ .init = bfa_fcs_lport_n2n_init,
+ .online = bfa_fcs_lport_n2n_online,
+ .offline = bfa_fcs_lport_n2n_offline
+ },
+ {
+ .init = bfa_fcs_lport_loop_init,
+ .online = bfa_fcs_lport_loop_online,
+ .offline = bfa_fcs_lport_loop_offline
+ },
+};
/*
* fcs_port_sm FCS logical port state machine
diff --git a/drivers/scsi/bfa/bfa_modules.h b/drivers/scsi/bfa/bfa_modules.h
index 53135f21fa0e..640621b4c3da 100644
--- a/drivers/scsi/bfa/bfa_modules.h
+++ b/drivers/scsi/bfa/bfa_modules.h
@@ -79,12 +79,12 @@ enum {
\
extern struct bfa_module_s hal_mod_ ## __mod; \
struct bfa_module_s hal_mod_ ## __mod = { \
- bfa_ ## __mod ## _meminfo, \
- bfa_ ## __mod ## _attach, \
- bfa_ ## __mod ## _detach, \
- bfa_ ## __mod ## _start,\
- bfa_ ## __mod ## _stop, \
- bfa_ ## __mod ## _iocdisable, \
+ .meminfo = bfa_ ## __mod ## _meminfo, \
+ .attach = bfa_ ## __mod ## _attach, \
+ .detach = bfa_ ## __mod ## _detach, \
+ .start = bfa_ ## __mod ## _start, \
+ .stop = bfa_ ## __mod ## _stop, \
+ .iocdisable = bfa_ ## __mod ## _iocdisable, \
}
#define BFA_CACHELINE_SZ (256)
--
2.7.4
--
Kees Cook
Nexus Security
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] cciss: use designated initializers
Prepare to mark sensitive kernel structures for randomization by making
sure they're using designated initializers. These were identified during
allyesconfig builds of x86, arm, and arm64, with most initializer fixes
extracted from grsecurity.
Signed-off-by: Kees Cook
---
drivers/block/cciss.h | 30 +++---
1 file changed, 15 insertions(+), 15 deletions(-)
diff --git a/drivers/block/cciss.h b/drivers/block/cciss.h
index 7fda30e4a241..428766d1ce80 100644
--- a/drivers/block/cciss.h
+++ b/drivers/block/cciss.h
@@ -402,27 +402,27 @@ static bool SA5_performant_intr_pending(ctlr_info_t *h)
}
static struct access_method SA5_access = {
- SA5_submit_command,
- SA5_intr_mask,
- SA5_fifo_full,
- SA5_intr_pending,
- SA5_completed,
+ .submit_command = SA5_submit_command,
+ .set_intr_mask = SA5_intr_mask,
+ .fifo_full = SA5_fifo_full,
+ .intr_pending = SA5_intr_pending,
+ .command_completed = SA5_completed,
};
static struct access_method SA5B_access = {
-SA5_submit_command,
-SA5B_intr_mask,
-SA5_fifo_full,
-SA5B_intr_pending,
-SA5_completed,
+ .submit_command = SA5_submit_command,
+ .set_intr_mask = SA5B_intr_mask,
+ .fifo_full = SA5_fifo_full,
+ .intr_pending = SA5B_intr_pending,
+ .command_completed = SA5_completed,
};
static struct access_method SA5_performant_access = {
- SA5_submit_command,
- SA5_performant_intr_mask,
- SA5_fifo_full,
- SA5_performant_intr_pending,
- SA5_performant_completed,
+ .submit_command = SA5_submit_command,
+ .set_intr_mask = SA5_performant_intr_mask,
+ .fifo_full = SA5_fifo_full,
+ .intr_pending = SA5_performant_intr_pending,
+ .command_completed = SA5_performant_completed,
};
struct board_type {
--
2.7.4
--
Kees Cook
Nexus Security
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] hpsa: use designated initializers
Prepare to mark sensitive kernel structures for randomization by making
sure they're using designated initializers. These were identified during
allyesconfig builds of x86, arm, and arm64, with most initializer fixes
extracted from grsecurity.
Signed-off-by: Kees Cook
---
drivers/scsi/hpsa.h | 40
1 file changed, 20 insertions(+), 20 deletions(-)
diff --git a/drivers/scsi/hpsa.h b/drivers/scsi/hpsa.h
index 64e98295b707..bf6cdc106654 100644
--- a/drivers/scsi/hpsa.h
+++ b/drivers/scsi/hpsa.h
@@ -578,38 +578,38 @@ static unsigned long SA5_ioaccel_mode1_completed(struct
ctlr_info *h, u8 q)
}
static struct access_method SA5_access = {
- SA5_submit_command,
- SA5_intr_mask,
- SA5_intr_pending,
- SA5_completed,
+ .submit_command = SA5_submit_command,
+ .set_intr_mask = SA5_intr_mask,
+ .intr_pending = SA5_intr_pending,
+ .command_completed = SA5_completed,
};
static struct access_method SA5_ioaccel_mode1_access = {
- SA5_submit_command,
- SA5_performant_intr_mask,
- SA5_ioaccel_mode1_intr_pending,
- SA5_ioaccel_mode1_completed,
+ .submit_command = SA5_submit_command,
+ .set_intr_mask = SA5_performant_intr_mask,
+ .intr_pending = SA5_ioaccel_mode1_intr_pending,
+ .command_completed = SA5_ioaccel_mode1_completed,
};
static struct access_method SA5_ioaccel_mode2_access = {
- SA5_submit_command_ioaccel2,
- SA5_performant_intr_mask,
- SA5_performant_intr_pending,
- SA5_performant_completed,
+ .submit_command = SA5_submit_command_ioaccel2,
+ .set_intr_mask = SA5_performant_intr_mask,
+ .intr_pending = SA5_performant_intr_pending,
+ .command_completed = SA5_performant_completed,
};
static struct access_method SA5_performant_access = {
- SA5_submit_command,
- SA5_performant_intr_mask,
- SA5_performant_intr_pending,
- SA5_performant_completed,
+ .submit_command = SA5_submit_command,
+ .set_intr_mask = SA5_performant_intr_mask,
+ .intr_pending = SA5_performant_intr_pending,
+ .command_completed = SA5_performant_completed,
};
static struct access_method SA5_performant_access_no_read = {
- SA5_submit_command_no_read,
- SA5_performant_intr_mask,
- SA5_performant_intr_pending,
- SA5_performant_completed,
+ .submit_command = SA5_submit_command_no_read,
+ .set_intr_mask = SA5_performant_intr_mask,
+ .intr_pending = SA5_performant_intr_pending,
+ .command_completed = SA5_performant_completed,
};
struct board_type {
--
2.7.4
--
Kees Cook
Nexus Security
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] scsi/bfa: use designated initializers
On Wed, Dec 21, 2016 at 12:33 AM, Christoph Hellwig wrote: > On Fri, Dec 16, 2016 at 05:05:15PM -0800, Kees Cook wrote: >> Prepare to mark sensitive kernel structures for randomization by making >> sure they're using designated initializers. These were identified during >> allyesconfig builds of x86, arm, and arm64, with most initializer fixes >> extracted from grsecurity. > > Instead of further bloating the idiotic dispatch table just kill it off > entirely: Sounds fine to me! Is this going via your tree? Thanks! -Kees -- Kees Cook Nexus Security -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2] scsi: esas2r: Fix format string type mistakes
From: Emese Revfy
This adds the missing __printf attribute which allows compile time
format string checking (and will be used by the coming initify gcc
plugin). Additionally, this fixes the warnings exposed by the attribute.
Signed-off-by: Emese Revfy
[kees: split scsi/acpi, merged attr and fix, new commit messages]
Signed-off-by: Kees Cook
---
v2:
- fix %lu to %zu on sizeof() values, bart
---
drivers/scsi/esas2r/esas2r_init.c | 2 +-
drivers/scsi/esas2r/esas2r_ioctl.c | 2 +-
drivers/scsi/esas2r/esas2r_log.h | 4 ++--
drivers/scsi/esas2r/esas2r_main.c | 4 ++--
4 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/scsi/esas2r/esas2r_init.c
b/drivers/scsi/esas2r/esas2r_init.c
index d6e53aee2295..6432a50b26d8 100644
--- a/drivers/scsi/esas2r/esas2r_init.c
+++ b/drivers/scsi/esas2r/esas2r_init.c
@@ -237,7 +237,7 @@ static void esas2r_claim_interrupts(struct esas2r_adapter
*a)
flags |= IRQF_SHARED;
esas2r_log(ESAS2R_LOG_INFO,
- "esas2r_claim_interrupts irq=%d (%p, %s, %x)",
+ "esas2r_claim_interrupts irq=%d (%p, %s, %lx)",
a->pcid->irq, a, a->name, flags);
if (request_irq(a->pcid->irq,
diff --git a/drivers/scsi/esas2r/esas2r_ioctl.c
b/drivers/scsi/esas2r/esas2r_ioctl.c
index 3e8483410f61..b35ed3829421 100644
--- a/drivers/scsi/esas2r/esas2r_ioctl.c
+++ b/drivers/scsi/esas2r/esas2r_ioctl.c
@@ -1301,7 +1301,7 @@ int esas2r_ioctl_handler(void *hostdata, int cmd, void
__user *arg)
ioctl = kzalloc(sizeof(struct atto_express_ioctl), GFP_KERNEL);
if (ioctl == NULL) {
esas2r_log(ESAS2R_LOG_WARN,
- "ioctl_handler kzalloc failed for %d bytes",
+ "ioctl_handler kzalloc failed for %zu bytes",
sizeof(struct atto_express_ioctl));
return -ENOMEM;
}
diff --git a/drivers/scsi/esas2r/esas2r_log.h b/drivers/scsi/esas2r/esas2r_log.h
index 7b6397bb5b94..75b9d23cd736 100644
--- a/drivers/scsi/esas2r/esas2r_log.h
+++ b/drivers/scsi/esas2r/esas2r_log.h
@@ -61,8 +61,8 @@ enum {
#endif
};
-int esas2r_log(const long level, const char *format, ...);
-int esas2r_log_dev(const long level,
+__printf(2, 3) int esas2r_log(const long level, const char *format, ...);
+__printf(3, 4) int esas2r_log_dev(const long level,
const struct device *dev,
const char *format,
...);
diff --git a/drivers/scsi/esas2r/esas2r_main.c
b/drivers/scsi/esas2r/esas2r_main.c
index 5092c821d088..f2e9d8aa979c 100644
--- a/drivers/scsi/esas2r/esas2r_main.c
+++ b/drivers/scsi/esas2r/esas2r_main.c
@@ -198,7 +198,7 @@ static ssize_t write_hw(struct file *file, struct kobject
*kobj,
GFP_KERNEL);
if (a->local_atto_ioctl == NULL) {
esas2r_log(ESAS2R_LOG_WARN,
- "write_hw kzalloc failed for %d bytes",
+ "write_hw kzalloc failed for %zu bytes",
sizeof(struct atto_ioctl));
return -ENOMEM;
}
@@ -1186,7 +1186,7 @@ static int esas2r_dev_targ_reset(struct scsi_cmnd *cmd,
bool target_reset)
} else {
esas2r_log(ESAS2R_LOG_CRIT,
"unable to allocate a request for a "
- "device reset (%d:%d)!",
+ "device reset (%d:%llu)!",
cmd->device->id,
cmd->device->lun);
}
--
2.7.4
--
Kees Cook
Nexus Security
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] cdrom: Make device operations read-only
Since function tables are a common target for attackers, it's best to keep
them in read-only memory. As such, this makes the CDROM device ops tables
const. This drops additionally n_minors, since it isn't used meaningfully,
and sets the only user of cdrom_dummy_generic_packet explicitly so the
variables can all be const.
Inspired by similar changes in grsecurity/PaX.
Signed-off-by: Kees Cook
---
Documentation/cdrom/cdrom-standard.tex | 9 +-
drivers/block/paride/pcd.c | 2 +-
drivers/cdrom/cdrom.c | 58 --
drivers/cdrom/gdrom.c | 4 +--
drivers/ide/ide-cd.c | 2 +-
drivers/scsi/sr.c | 2 +-
include/linux/cdrom.h | 5 +--
7 files changed, 37 insertions(+), 45 deletions(-)
diff --git a/Documentation/cdrom/cdrom-standard.tex
b/Documentation/cdrom/cdrom-standard.tex
index c06233fe52ac..8f85b0e41046 100644
--- a/Documentation/cdrom/cdrom-standard.tex
+++ b/Documentation/cdrom/cdrom-standard.tex
@@ -249,7 +249,6 @@ struct& cdrom_device_ops\ \{ \hidewidth\cr
unsigned\ long);\cr
\noalign{\medskip}
&const\ int& capability;& capability flags \cr
- &int& n_minors;& number of active minor devices \cr
\};\cr
}
$$
@@ -258,13 +257,7 @@ it should add a function pointer to this $struct$. When a
particular
function is not implemented, however, this $struct$ should contain a
NULL instead. The $capability$ flags specify the capabilities of the
\cdrom\ hardware and/or low-level \cdrom\ driver when a \cdrom\ drive
-is registered with the \UCD. The value $n_minors$ should be a positive
-value indicating the number of minor devices that are supported by
-the low-level device driver, normally~1. Although these two variables
-are `informative' rather than `operational,' they are included in
-$cdrom_device_ops$ because they describe the capability of the {\em
-driver\/} rather than the {\em drive}. Nomenclature has always been
-difficult in computer programming.
+is registered with the \UCD.
Note that most functions have fewer parameters than their
$blkdev_fops$ counterparts. This is because very little of the
diff --git a/drivers/block/paride/pcd.c b/drivers/block/paride/pcd.c
index 5fd2d0e25567..10aed84244f5 100644
--- a/drivers/block/paride/pcd.c
+++ b/drivers/block/paride/pcd.c
@@ -273,7 +273,7 @@ static const struct block_device_operations pcd_bdops = {
.check_events = pcd_block_check_events,
};
-static struct cdrom_device_ops pcd_dops = {
+static const struct cdrom_device_ops pcd_dops = {
.open = pcd_open,
.release= pcd_release,
.drive_status = pcd_drive_status,
diff --git a/drivers/cdrom/cdrom.c b/drivers/cdrom/cdrom.c
index 59cca72647a6..bbbd3caa927c 100644
--- a/drivers/cdrom/cdrom.c
+++ b/drivers/cdrom/cdrom.c
@@ -342,8 +342,8 @@ static void cdrom_sysctl_register(void);
static LIST_HEAD(cdrom_list);
-static int cdrom_dummy_generic_packet(struct cdrom_device_info *cdi,
- struct packet_command *cgc)
+int cdrom_dummy_generic_packet(struct cdrom_device_info *cdi,
+ struct packet_command *cgc)
{
if (cgc->sense) {
cgc->sense->sense_key = 0x05;
@@ -354,6 +354,7 @@ static int cdrom_dummy_generic_packet(struct
cdrom_device_info *cdi,
cgc->stat = -EIO;
return -EIO;
}
+EXPORT_SYMBOL(cdrom_dummy_generic_packet);
static int cdrom_flush_cache(struct cdrom_device_info *cdi)
{
@@ -371,7 +372,7 @@ static int cdrom_flush_cache(struct cdrom_device_info *cdi)
static int cdrom_get_disc_info(struct cdrom_device_info *cdi,
disc_information *di)
{
- struct cdrom_device_ops *cdo = cdi->ops;
+ const struct cdrom_device_ops *cdo = cdi->ops;
struct packet_command cgc;
int ret, buflen;
@@ -586,7 +587,7 @@ static int cdrom_mrw_set_lba_space(struct cdrom_device_info
*cdi, int space)
int register_cdrom(struct cdrom_device_info *cdi)
{
static char banner_printed;
- struct cdrom_device_ops *cdo = cdi->ops;
+ const struct cdrom_device_ops *cdo = cdi->ops;
int *change_capability = (int *)&cdo->capability; /* hack */
cd_dbg(CD_OPEN, "entering register_cdrom\n");
@@ -610,7 +611,6 @@ int register_cdrom(struct cdrom_device_info *cdi)
ENSURE(reset, CDC_RESET);
ENSURE(generic_packet, CDC_GENERIC_PACKET);
cdi->mc_flags = 0;
- cdo->n_minors = 0;
cdi->options = CDO_USE_FFLAGS;
if (autoclose == 1 && CDROM_CAN(CDC_CLOSE_TRAY))
@@ -630,8 +630,7 @@ int register_cdrom(struct cdrom_device_info *cdi)
else
cdi->cdda_method = CDDA_OLD;
- if (!cdo->generic_packet)
- cdo->generic_packet = cdrom_dummy_generic_packet
[PATCH] scsi: esas2r: fix potential format string flaw
This makes sure format strings cannot leak into the printk call via the
constructed buffer.
Signed-off-by: Kees Cook
---
drivers/scsi/esas2r/esas2r_log.c |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/scsi/esas2r/esas2r_log.c b/drivers/scsi/esas2r/esas2r_log.c
index 9bf285d..61fc19d 100644
--- a/drivers/scsi/esas2r/esas2r_log.c
+++ b/drivers/scsi/esas2r/esas2r_log.c
@@ -171,7 +171,7 @@ static int esas2r_log_master(const long level,
if (strlen(event_buffer) < buflen)
strcat(buffer, "\n");
- printk(event_buffer);
+ printk("%s", event_buffer);
spin_unlock_irqrestore(&event_buffer_lock, flags);
}
--
1.7.9.5
--
Kees Cook
Chrome OS Security
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch] [SCSI] megaraid: missing bounds check in mimd_to_kioc()
Hi,
On Wed, Oct 30, 2013 at 10:13 AM, Dan Carpenter
wrote:
> pthru32->dataxferlen comes from the user so we need to check that it's
> not too large so we don't overflow the buffer.
>
> Reported-by: Nico Golde
> Reported-by: Fabian Yamaguchi
> Signed-off-by: Dan Carpenter
I don't see this in Linus's tree nor linux-next yet. Can someone pick this up?
Thanks!
-Kees
> ---
> Please review this carefully because I have not tested it.
>
> diff --git a/drivers/scsi/megaraid/megaraid_mm.c
> b/drivers/scsi/megaraid/megaraid_mm.c
> index dfffd0f..a706927 100644
> --- a/drivers/scsi/megaraid/megaraid_mm.c
> +++ b/drivers/scsi/megaraid/megaraid_mm.c
> @@ -486,6 +486,8 @@ mimd_to_kioc(mimd_t __user *umimd, mraid_mmadp_t *adp,
> uioc_t *kioc)
>
> pthru32->dataxferaddr = kioc->buf_paddr;
> if (kioc->data_dir & UIOC_WR) {
> + if (pthru32->dataxferlen > kioc->xferlen)
> + return -EINVAL;
> if (copy_from_user(kioc->buf_vaddr, kioc->user_data,
> pthru32->dataxferlen)) {
> return (-EFAULT);
>
--
Kees Cook
Chrome OS Security
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[RESEND][PATCH] scsi: esas2r: fix potential format string flaw
This makes sure format strings cannot leak into the printk call via the
constructed buffer.
Signed-off-by: Kees Cook
---
drivers/scsi/esas2r/esas2r_log.c |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/scsi/esas2r/esas2r_log.c b/drivers/scsi/esas2r/esas2r_log.c
index 9bf285df58dd..61fc19d296bd 100644
--- a/drivers/scsi/esas2r/esas2r_log.c
+++ b/drivers/scsi/esas2r/esas2r_log.c
@@ -171,7 +171,7 @@ static int esas2r_log_master(const long level,
if (strlen(event_buffer) < buflen)
strcat(buffer, "\n");
- printk(event_buffer);
+ printk("%s", event_buffer);
spin_unlock_irqrestore(&event_buffer_lock, flags);
}
--
1.7.9.5
--
Kees Cook
Chrome OS Security
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RESEND][PATCH] scsi: esas2r: fix potential format string flaw
On Tue, Dec 17, 2013 at 12:00 PM, Greg Kroah-Hartman wrote: > On Tue, Dec 17, 2013 at 10:27:33AM -0800, Kees Cook wrote: >> This makes sure format strings cannot leak into the printk call via the >> constructed buffer. >> >> Signed-off-by: Kees Cook >> --- >> drivers/scsi/esas2r/esas2r_log.c |2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) > > Why is this patch "To:" me? I'm not the author of this driver, or the > maintainer of it or the subsystem, and there's not much, if anything I > can do with it... I've resent this before, and since it lived in "drivers", I figured you would be the next up the chain to take it (since it's been ignored). -Kees -- Kees Cook Chrome OS Security -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RESEND][PATCH] scsi: esas2r: fix potential format string flaw
On Tue, Dec 17, 2013 at 9:42 PM, Joe Perches wrote:
> On Tue, 2013-12-17 at 10:27 -0800, Kees Cook wrote:
>> This makes sure format strings cannot leak into the printk call via the
>> constructed buffer.
> []
>> diff --git a/drivers/scsi/esas2r/esas2r_log.c
>> b/drivers/scsi/esas2r/esas2r_log.c
> []
>> @@ -171,7 +171,7 @@ static int esas2r_log_master(const long level,
>> if (strlen(event_buffer) < buflen)
>> strcat(buffer, "\n");
>>
>> - printk(event_buffer);
>> + printk("%s", event_buffer);
>
> It's probably better to remove the
>
> if (strlen(event_buffer) < buflen)
> strcat(buffer, "\n");
>
> and use
>
> printk("%s\n", event_buffer);
>
> so that the output is always newline terminated.
Ah! Yes, good call.
-Kees
--
Kees Cook
Chrome OS Security
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2] scsi: esas2r: fix potential format string flaw
This makes sure format strings cannot leak into the printk call via the
constructed buffer.
Signed-off-by: Kees Cook
Acked-by: Bradley Grove
---
v2:
- add newline via printk instead.
---
drivers/scsi/esas2r/esas2r_log.c |8 ++--
1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/drivers/scsi/esas2r/esas2r_log.c b/drivers/scsi/esas2r/esas2r_log.c
index 9bf285df58dd..a82030aa8577 100644
--- a/drivers/scsi/esas2r/esas2r_log.c
+++ b/drivers/scsi/esas2r/esas2r_log.c
@@ -165,13 +165,9 @@ static int esas2r_log_master(const long level,
/*
* Put a line break at the end of the formatted string so that
-* we don't wind up with run-on messages. only append if there
-* is enough space in the buffer.
+* we don't wind up with run-on messages.
*/
- if (strlen(event_buffer) < buflen)
- strcat(buffer, "\n");
-
- printk(event_buffer);
+ printk("%s\n", event_buffer);
spin_unlock_irqrestore(&event_buffer_lock, flags);
}
--
1.7.9.5
--
Kees Cook
Chrome OS Security
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch] [SCSI] megaraid: missing bounds check in mimd_to_kioc()
On Wed, Jan 8, 2014 at 4:27 AM, Saxena, Sumit wrote:
>
>
>>-Original Message-
>>From: Dan Carpenter [mailto:dan.carpen...@oracle.com]
>>Sent: Wednesday, October 30, 2013 10:44 PM
>>To: DL-MegaRAID Linux
>>Cc: James E.J. Bottomley; linux-scsi@vger.kernel.org; secur...@kernel.org;
>>Nico Golde; Fabian Yamaguchi
>>Subject: [patch] [SCSI] megaraid: missing bounds check in mimd_to_kioc()
>>
>>pthru32->dataxferlen comes from the user so we need to check that it's
>>not too large so we don't overflow the buffer.
>>
>>Reported-by: Nico Golde
>>Reported-by: Fabian Yamaguchi
>>Signed-off-by: Dan Carpenter
>>---
>>Please review this carefully because I have not tested it.
>>
>>diff --git a/drivers/scsi/megaraid/megaraid_mm.c
>>b/drivers/scsi/megaraid/megaraid_mm.c
>>index dfffd0f..a706927 100644
>>--- a/drivers/scsi/megaraid/megaraid_mm.c
>>+++ b/drivers/scsi/megaraid/megaraid_mm.c
>>@@ -486,6 +486,8 @@ mimd_to_kioc(mimd_t __user *umimd,
>>mraid_mmadp_t *adp, uioc_t *kioc)
>>
>> pthru32->dataxferaddr = kioc->buf_paddr;
>> if (kioc->data_dir & UIOC_WR) {
>>+ if (pthru32->dataxferlen > kioc->xferlen)
>>+ return -EINVAL;
>> if (copy_from_user(kioc->buf_vaddr, kioc->user_data,
>> pthru32->dataxferlen)) {
>> return (-EFAULT);
>
> Acked-by: Sumit Saxena
>
> Sumit
>
Thanks for the Ack. Who normally picks patches for this area?
-Kees
--
Kees Cook
Chrome OS Security
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch] [SCSI] megaraid: missing bounds check in mimd_to_kioc()
On Thu, Jan 9, 2014 at 11:53 AM, Saxena, Sumit wrote:
>
>
>>-Original Message-----
>>From: Kees Cook [mailto:keesc...@google.com]
>>Sent: Friday, January 10, 2014 12:05 AM
>>To: Saxena, Sumit
>>Cc: Dan Carpenter; DL-MegaRAID Linux; James E.J. Bottomley; linux-
>>s...@vger.kernel.org; secur...@kernel.org; Nico Golde; Fabian Yamaguchi
>>Subject: Re: [patch] [SCSI] megaraid: missing bounds check in mimd_to_kioc()
>>
>>On Wed, Jan 8, 2014 at 4:27 AM, Saxena, Sumit
>>wrote:
>>>
>>>
>>>>-Original Message-
>>>>From: Dan Carpenter [mailto:dan.carpen...@oracle.com]
>>>>Sent: Wednesday, October 30, 2013 10:44 PM
>>>>To: DL-MegaRAID Linux
>>>>Cc: James E.J. Bottomley; linux-scsi@vger.kernel.org;
>>>>secur...@kernel.org; Nico Golde; Fabian Yamaguchi
>>>>Subject: [patch] [SCSI] megaraid: missing bounds check in
>>>>mimd_to_kioc()
>>>>
>>>>pthru32->dataxferlen comes from the user so we need to check that it's
>>>>not too large so we don't overflow the buffer.
>>>>
>>>>Reported-by: Nico Golde
>>>>Reported-by: Fabian Yamaguchi
>>>>Signed-off-by: Dan Carpenter
>>>>---
>>>>Please review this carefully because I have not tested it.
>>>>
>>>>diff --git a/drivers/scsi/megaraid/megaraid_mm.c
>>>>b/drivers/scsi/megaraid/megaraid_mm.c
>>>>index dfffd0f..a706927 100644
>>>>--- a/drivers/scsi/megaraid/megaraid_mm.c
>>>>+++ b/drivers/scsi/megaraid/megaraid_mm.c
>>>>@@ -486,6 +486,8 @@ mimd_to_kioc(mimd_t __user *umimd,
>>mraid_mmadp_t
>>>>*adp, uioc_t *kioc)
>>>>
>>>> pthru32->dataxferaddr = kioc->buf_paddr;
>>>> if (kioc->data_dir & UIOC_WR) {
>>>>+ if (pthru32->dataxferlen > kioc->xferlen)
>>>>+ return -EINVAL;
>>>> if (copy_from_user(kioc->buf_vaddr, kioc->user_data,
>>>> pthru32->dataxferlen)) {
>>>> return (-EFAULT);
>>>
>>> Acked-by: Sumit Saxena
>>>
>>> Sumit
>>>
>>
>>Thanks for the Ack. Who normally picks patches for this area?
>>
>>-Kees
>>
> James Bottomley(Linux SCSI subsystem maintainer) should pick this patch.
Okay, thanks. James, can you pick up this patch as well?
https://lkml.org/lkml/2013/12/18/477
Thanks,
-Kees
>
> Sumit
>>--
>>Kees Cook
>>Chrome OS Security
>
--
Kees Cook
Chrome OS Security
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 19/36] scsi: Define usercopy region in scsi_sense_cache slab cache
From: David Windsor
SCSI sense buffers, stored in struct scsi_cmnd.sense and therefore
contained in the scsi_sense_cache slab cache, need to be copied to/from
userspace.
cache object allocation:
drivers/scsi/scsi_lib.c:
scsi_select_sense_cache(...):
return ... ? scsi_sense_isadma_cache : scsi_sense_cache
scsi_alloc_sense_buffer(...):
return kmem_cache_alloc_node(scsi_select_sense_cache(), ...);
scsi_init_request(...):
...
cmd->sense_buffer = scsi_alloc_sense_buffer(...);
...
cmd->req.sense = cmd->sense_buffer
example usage trace:
block/scsi_ioctl.c:
(inline from sg_io)
blk_complete_sghdr_rq(...):
struct scsi_request *req = scsi_req(rq);
...
copy_to_user(..., req->sense, len)
scsi_cmd_ioctl(...):
sg_io(...);
In support of usercopy hardening, this patch defines a region in
the scsi_sense_cache slab cache in which userspace copy operations
are allowed.
This region is known as the slab cache's usercopy region. Slab caches
can now check that each dynamically sized copy operation involving
cache-managed memory falls entirely within the slab's usercopy region.
Signed-off-by: David Windsor
[kees: adjust commit log, provide usage trace]
Cc: "James E.J. Bottomley"
Cc: "Martin K. Petersen"
Cc: linux-scsi@vger.kernel.org
Signed-off-by: Kees Cook
---
drivers/scsi/scsi_lib.c | 9 +
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 1cbc497e00bd..164d062c4d94 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -79,14 +79,15 @@ int scsi_init_sense_cache(struct Scsi_Host *shost)
if (shost->unchecked_isa_dma) {
scsi_sense_isadma_cache =
kmem_cache_create("scsi_sense_cache(DMA)",
- SCSI_SENSE_BUFFERSIZE, 0,
- SLAB_HWCACHE_ALIGN | SLAB_CACHE_DMA, NULL);
+ SCSI_SENSE_BUFFERSIZE, 0,
+ SLAB_HWCACHE_ALIGN | SLAB_CACHE_DMA, NULL);
if (!scsi_sense_isadma_cache)
ret = -ENOMEM;
} else {
scsi_sense_cache =
- kmem_cache_create("scsi_sense_cache",
- SCSI_SENSE_BUFFERSIZE, 0, SLAB_HWCACHE_ALIGN, NULL);
+ kmem_cache_create_usercopy("scsi_sense_cache",
+ SCSI_SENSE_BUFFERSIZE, 0, SLAB_HWCACHE_ALIGN,
+ 0, SCSI_SENSE_BUFFERSIZE, NULL);
if (!scsi_sense_cache)
ret = -ENOMEM;
}
--
2.7.4
[PATCH 22/38] scsi: Define usercopy region in scsi_sense_cache slab cache
From: David Windsor
SCSI sense buffers, stored in struct scsi_cmnd.sense and therefore
contained in the scsi_sense_cache slab cache, need to be copied to/from
userspace.
cache object allocation:
drivers/scsi/scsi_lib.c:
scsi_select_sense_cache(...):
return ... ? scsi_sense_isadma_cache : scsi_sense_cache
scsi_alloc_sense_buffer(...):
return kmem_cache_alloc_node(scsi_select_sense_cache(), ...);
scsi_init_request(...):
...
cmd->sense_buffer = scsi_alloc_sense_buffer(...);
...
cmd->req.sense = cmd->sense_buffer
example usage trace:
block/scsi_ioctl.c:
(inline from sg_io)
blk_complete_sghdr_rq(...):
struct scsi_request *req = scsi_req(rq);
...
copy_to_user(..., req->sense, len)
scsi_cmd_ioctl(...):
sg_io(...);
In support of usercopy hardening, this patch defines a region in
the scsi_sense_cache slab cache in which userspace copy operations
are allowed.
This region is known as the slab cache's usercopy region. Slab caches
can now check that each dynamically sized copy operation involving
cache-managed memory falls entirely within the slab's usercopy region.
Signed-off-by: David Windsor
[kees: adjust commit log, provide usage trace]
Cc: "James E.J. Bottomley"
Cc: "Martin K. Petersen"
Cc: linux-scsi@vger.kernel.org
Signed-off-by: Kees Cook
---
drivers/scsi/scsi_lib.c | 9 +
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 1cbc497e00bd..164d062c4d94 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -79,14 +79,15 @@ int scsi_init_sense_cache(struct Scsi_Host *shost)
if (shost->unchecked_isa_dma) {
scsi_sense_isadma_cache =
kmem_cache_create("scsi_sense_cache(DMA)",
- SCSI_SENSE_BUFFERSIZE, 0,
- SLAB_HWCACHE_ALIGN | SLAB_CACHE_DMA, NULL);
+ SCSI_SENSE_BUFFERSIZE, 0,
+ SLAB_HWCACHE_ALIGN | SLAB_CACHE_DMA, NULL);
if (!scsi_sense_isadma_cache)
ret = -ENOMEM;
} else {
scsi_sense_cache =
- kmem_cache_create("scsi_sense_cache",
- SCSI_SENSE_BUFFERSIZE, 0, SLAB_HWCACHE_ALIGN, NULL);
+ kmem_cache_create_usercopy("scsi_sense_cache",
+ SCSI_SENSE_BUFFERSIZE, 0, SLAB_HWCACHE_ALIGN,
+ 0, SCSI_SENSE_BUFFERSIZE, NULL);
if (!scsi_sense_cache)
ret = -ENOMEM;
}
--
2.7.4
Re: [PATCH] aic7xxx/aic79xx: remove VLAs
On Thu, Mar 8, 2018 at 5:22 AM, Stephen Kitt wrote:
> In preparation to enabling -Wvla, remove VLAs and replace them with
> fixed-length arrays instead.
>
> The arrays fixed here, using the number of constant sections, aren't
> really VLAs, but they appear so to the compiler. Since we know at
> build-time how many critical sections there are, we might as well use
> a pre-processor-level constant instead.
>
> This was prompted by https://lkml.org/lkml/2018/3/7/621
>
> Signed-off-by: Stephen Kitt
> ---
> drivers/scsi/aic7xxx/aic79xx_core.c| 8
> drivers/scsi/aic7xxx/aic79xx_seq.h_shipped | 3 +--
> drivers/scsi/aic7xxx/aic7xxx_core.c| 8
> drivers/scsi/aic7xxx/aic7xxx_seq.h_shipped | 3 +--
> drivers/scsi/aic7xxx/aicasm/aicasm.c | 6 --
> 5 files changed, 14 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/scsi/aic7xxx/aic79xx_core.c
> b/drivers/scsi/aic7xxx/aic79xx_core.c
> index b560f396ee99..034f4eebb160 100644
> --- a/drivers/scsi/aic7xxx/aic79xx_core.c
> +++ b/drivers/scsi/aic7xxx/aic79xx_core.c
> @@ -9338,9 +9338,9 @@ ahd_dumpseq(struct ahd_softc* ahd)
> static void
> ahd_loadseq(struct ahd_softc *ahd)
> {
> - struct cs cs_table[num_critical_sections];
> - u_int begin_set[num_critical_sections];
> - u_int end_set[num_critical_sections];
> + struct cs cs_table[NUM_CRITICAL_SECTIONS];
> + u_int begin_set[NUM_CRITICAL_SECTIONS];
> + u_int end_set[NUM_CRITICAL_SECTIONS];
> const struct patch *cur_patch;
> u_int cs_count;
> u_int cur_cs;
> @@ -9456,7 +9456,7 @@ ahd_loadseq(struct ahd_softc *ahd)
> * Move through the CS table until we find a CS
> * that might apply to this instruction.
> */
> - for (; cur_cs < num_critical_sections; cur_cs++) {
> + for (; cur_cs < NUM_CRITICAL_SECTIONS; cur_cs++) {
> if (critical_sections[cur_cs].end <= i) {
> if (begin_set[cs_count] == TRUE
> && end_set[cs_count] == FALSE) {
> diff --git a/drivers/scsi/aic7xxx/aic79xx_seq.h_shipped
> b/drivers/scsi/aic7xxx/aic79xx_seq.h_shipped
> index 4b51e232392f..20fb9ca9e271 100644
> --- a/drivers/scsi/aic7xxx/aic79xx_seq.h_shipped
> +++ b/drivers/scsi/aic7xxx/aic79xx_seq.h_shipped
> @@ -1186,5 +1186,4 @@ static const struct cs {
> { 759, 763 }
> };
>
> -static const int num_critical_sections = sizeof(critical_sections)
> - / sizeof(*critical_sections);
> +#define NUM_CRITICAL_SECTIONS 14
The compiler doesn't treat "const" as a literal, hence the need to
change this. However, you can still use the sizeof (actually, this is
exactly ARRAY_SIZE()). Perhaps:
#define NUM_CRITICAL_SECTIONS ARRAY_SIZE(critical_sections)
?
Otherwise, looks great!
-Kees
--
Kees Cook
Pixel Security
Re: [PATCH v2] aic7xxx/aic79xx: remove VLAs
On Thu, Mar 8, 2018 at 12:51 PM, Stephen Kitt wrote:
> In preparation to enabling -Wvla, remove VLAs and replace them with
> fixed-length arrays instead.
>
> The arrays fixed here, using the number of constant sections, aren't
> really VLAs, but they appear so to the compiler. Replace the array
> sizes with a pre-processor-level constant instead using ARRAY_SIZE.
>
> This was prompted by https://lkml.org/lkml/2018/3/7/621
>
> Signed-off-by: Stephen Kitt
Thanks!
Reviewed-by: Kees Cook
-Kees
> ---
> drivers/scsi/aic7xxx/aic79xx_core.c| 8
> drivers/scsi/aic7xxx/aic79xx_seq.h_shipped | 3 +--
> drivers/scsi/aic7xxx/aic7xxx_core.c| 8
> drivers/scsi/aic7xxx/aic7xxx_seq.h_shipped | 3 +--
> drivers/scsi/aic7xxx/aicasm/aicasm.c | 3 +--
> 5 files changed, 11 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/scsi/aic7xxx/aic79xx_core.c
> b/drivers/scsi/aic7xxx/aic79xx_core.c
> index b560f396ee99..034f4eebb160 100644
> --- a/drivers/scsi/aic7xxx/aic79xx_core.c
> +++ b/drivers/scsi/aic7xxx/aic79xx_core.c
> @@ -9338,9 +9338,9 @@ ahd_dumpseq(struct ahd_softc* ahd)
> static void
> ahd_loadseq(struct ahd_softc *ahd)
> {
> - struct cs cs_table[num_critical_sections];
> - u_int begin_set[num_critical_sections];
> - u_int end_set[num_critical_sections];
> + struct cs cs_table[NUM_CRITICAL_SECTIONS];
> + u_int begin_set[NUM_CRITICAL_SECTIONS];
> + u_int end_set[NUM_CRITICAL_SECTIONS];
> const struct patch *cur_patch;
> u_int cs_count;
> u_int cur_cs;
> @@ -9456,7 +9456,7 @@ ahd_loadseq(struct ahd_softc *ahd)
> * Move through the CS table until we find a CS
> * that might apply to this instruction.
> */
> - for (; cur_cs < num_critical_sections; cur_cs++) {
> + for (; cur_cs < NUM_CRITICAL_SECTIONS; cur_cs++) {
> if (critical_sections[cur_cs].end <= i) {
> if (begin_set[cs_count] == TRUE
> && end_set[cs_count] == FALSE) {
> diff --git a/drivers/scsi/aic7xxx/aic79xx_seq.h_shipped
> b/drivers/scsi/aic7xxx/aic79xx_seq.h_shipped
> index 4b51e232392f..fd64a950ee44 100644
> --- a/drivers/scsi/aic7xxx/aic79xx_seq.h_shipped
> +++ b/drivers/scsi/aic7xxx/aic79xx_seq.h_shipped
> @@ -1186,5 +1186,4 @@ static const struct cs {
> { 759, 763 }
> };
>
> -static const int num_critical_sections = sizeof(critical_sections)
> - / sizeof(*critical_sections);
> +#define NUM_CRITICAL_SECTIONS ARRAY_SIZE(critical_sections)
> diff --git a/drivers/scsi/aic7xxx/aic7xxx_core.c
> b/drivers/scsi/aic7xxx/aic7xxx_core.c
> index 6612ff3b2e83..e97eceacf522 100644
> --- a/drivers/scsi/aic7xxx/aic7xxx_core.c
> +++ b/drivers/scsi/aic7xxx/aic7xxx_core.c
> @@ -6848,9 +6848,9 @@ ahc_dumpseq(struct ahc_softc* ahc)
> static int
> ahc_loadseq(struct ahc_softc *ahc)
> {
> - struct cs cs_table[num_critical_sections];
> - u_int begin_set[num_critical_sections];
> - u_int end_set[num_critical_sections];
> + struct cs cs_table[NUM_CRITICAL_SECTIONS];
> + u_int begin_set[NUM_CRITICAL_SECTIONS];
> + u_int end_set[NUM_CRITICAL_SECTIONS];
> const struct patch *cur_patch;
> u_int cs_count;
> u_int cur_cs;
> @@ -6915,7 +6915,7 @@ ahc_loadseq(struct ahc_softc *ahc)
> * Move through the CS table until we find a CS
> * that might apply to this instruction.
> */
> - for (; cur_cs < num_critical_sections; cur_cs++) {
> + for (; cur_cs < NUM_CRITICAL_SECTIONS; cur_cs++) {
> if (critical_sections[cur_cs].end <= i) {
> if (begin_set[cs_count] == TRUE
> && end_set[cs_count] == FALSE) {
> diff --git a/drivers/scsi/aic7xxx/aic7xxx_seq.h_shipped
> b/drivers/scsi/aic7xxx/aic7xxx_seq.h_shipped
> index 07e93fbae706..f37362bc8ece 100644
> --- a/drivers/scsi/aic7xxx/aic7xxx_seq.h_shipped
> +++ b/drivers/scsi/aic7xxx/aic7xxx_seq.h_shipped
> @@ -1304,5 +1304,4 @@ static const struct cs {
> { 875, 877 }
> };
>
> -static const int num_critical_sections = sizeof(critical_sections)
> - / sizeof(*critical_sections);
> +#define NUM_CRITICAL_SECTIONS ARRAY_SIZE(critical_sections)
> diff --git a/drivers/scsi/aic7xxx/aicasm/aicasm.c
> b/drivers/scsi/aic7xxx/aicasm/aicasm.c
> index 21ac265280bf..5f474e490f3e 100644
> --- a/drivers/scsi
Re: [PATCH] bfa: remove VLA
On Thu, Mar 8, 2018 at 1:38 PM, Stephen Kitt wrote:
> In preparation to enabling -Wvla, remove VLAs and replace them with
> fixed-length arrays instead.
>
> bfad_bsg.c uses a variable-length array declaration to measure the
> size of a putative array; this can be replaced by the product of the
> size of an element and the number of elements, avoiding the VLA
> altogether.
>
> This was prompted by https://lkml.org/lkml/2018/3/7/621
>
> Signed-off-by: Stephen Kitt
Reviewed-by: Kees Cook
-Kees
> ---
> drivers/scsi/bfa/bfad_bsg.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/scsi/bfa/bfad_bsg.c b/drivers/scsi/bfa/bfad_bsg.c
> index 3976e787ba64..7c884f881180 100644
> --- a/drivers/scsi/bfa/bfad_bsg.c
> +++ b/drivers/scsi/bfa/bfad_bsg.c
> @@ -891,7 +891,7 @@ bfad_iocmd_fabric_get_lports(struct bfad_s *bfad, void
> *cmd,
>
> if (bfad_chk_iocmd_sz(payload_len,
> sizeof(struct bfa_bsg_fabric_get_lports_s),
> - sizeof(wwn_t[iocmd->nports])) != BFA_STATUS_OK) {
> + sizeof(wwn_t) * iocmd->nports) != BFA_STATUS_OK) {
> iocmd->status = BFA_STATUS_VERSION_FAIL;
> goto out;
> }
> --
> 2.11.0
>
--
Kees Cook
Pixel Security
Re: [PATCH 0/5] v3 block subsystem refcounter conversions
On Tue, Jun 27, 2017 at 6:26 AM, Jens Axboe wrote:
> On 06/27/2017 05:39 AM, Elena Reshetova wrote:
>> Changes in v3:
>> No changes in patches apart from trivial rebases, but now by
>> default refcount_t = atomic_t and uses all atomic standard operations
>> unless CONFIG_REFCOUNT_FULL is enabled. This is a compromize for the
>> systems that are critical on performance and cannot accept even
>> slight delay on the refcounter operations.
>
> Is that true in 4.12-rc, or is that true in a later release once
> Linus has pulled those changes in? If the latter, please resend
> this when those changes are in, thanks.
It's in -next currently ("locking/refcount: Create unchecked atomic_t
implementation")
-Kees
--
Kees Cook
Pixel Security
[PATCH v2 17/30] scsi: Define usercopy region in scsi_sense_cache slab cache
From: David Windsor
SCSI sense buffers, stored in struct scsi_cmnd.sense and therefore
contained in the scsi_sense_cache slab cache, need to be copied to/from
userspace.
cache object allocation:
drivers/scsi/scsi_lib.c:
scsi_select_sense_cache(...):
return ... ? scsi_sense_isadma_cache : scsi_sense_cache
scsi_alloc_sense_buffer(...):
return kmem_cache_alloc_node(scsi_select_sense_cache(), ...);
scsi_init_request(...):
...
cmd->sense_buffer = scsi_alloc_sense_buffer(...);
...
cmd->req.sense = cmd->sense_buffer
example usage trace:
block/scsi_ioctl.c:
(inline from sg_io)
blk_complete_sghdr_rq(...):
struct scsi_request *req = scsi_req(rq);
...
copy_to_user(..., req->sense, len)
scsi_cmd_ioctl(...):
sg_io(...);
In support of usercopy hardening, this patch defines a region in
the scsi_sense_cache slab cache in which userspace copy operations
are allowed.
This region is known as the slab cache's usercopy region. Slab
caches can now check that each copy operation involving cache-managed
memory falls entirely within the slab's usercopy region.
Signed-off-by: David Windsor
[kees: adjust commit log, provide usage trace]
Cc: "James E.J. Bottomley"
Cc: "Martin K. Petersen"
Cc: linux-scsi@vger.kernel.org
Signed-off-by: Kees Cook
---
drivers/scsi/scsi_lib.c | 9 +
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index f6097b89d5d3..f1c6bd56dd5b 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -77,14 +77,15 @@ int scsi_init_sense_cache(struct Scsi_Host *shost)
if (shost->unchecked_isa_dma) {
scsi_sense_isadma_cache =
kmem_cache_create("scsi_sense_cache(DMA)",
- SCSI_SENSE_BUFFERSIZE, 0,
- SLAB_HWCACHE_ALIGN | SLAB_CACHE_DMA, NULL);
+ SCSI_SENSE_BUFFERSIZE, 0,
+ SLAB_HWCACHE_ALIGN | SLAB_CACHE_DMA, NULL);
if (!scsi_sense_isadma_cache)
ret = -ENOMEM;
} else {
scsi_sense_cache =
- kmem_cache_create("scsi_sense_cache",
- SCSI_SENSE_BUFFERSIZE, 0, SLAB_HWCACHE_ALIGN, NULL);
+ kmem_cache_create_usercopy("scsi_sense_cache",
+ SCSI_SENSE_BUFFERSIZE, 0, SLAB_HWCACHE_ALIGN,
+ 0, SCSI_SENSE_BUFFERSIZE, NULL);
if (!scsi_sense_cache)
ret = -ENOMEM;
}
--
2.7.4
Re: [PATCH v2 17/30] scsi: Define usercopy region in scsi_sense_cache slab cache
On Mon, Aug 28, 2017 at 2:42 PM, Bart Van Assche wrote:
> On Mon, 2017-08-28 at 14:34 -0700, Kees Cook wrote:
>> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
>> index f6097b89d5d3..f1c6bd56dd5b 100644
>> --- a/drivers/scsi/scsi_lib.c
>> +++ b/drivers/scsi/scsi_lib.c
>> @@ -77,14 +77,15 @@ int scsi_init_sense_cache(struct Scsi_Host *shost)
>> if (shost->unchecked_isa_dma) {
>> scsi_sense_isadma_cache =
>> kmem_cache_create("scsi_sense_cache(DMA)",
>> - SCSI_SENSE_BUFFERSIZE, 0,
>> - SLAB_HWCACHE_ALIGN | SLAB_CACHE_DMA, NULL);
>> + SCSI_SENSE_BUFFERSIZE, 0,
>> + SLAB_HWCACHE_ALIGN | SLAB_CACHE_DMA, NULL);
>> if (!scsi_sense_isadma_cache)
>> ret = -ENOMEM;
>
> All this part of this patch does is to change source code indentation. Should
> these changes really be included in this patch?
I can certainly drop that hunk, but the existing alignment is really
ugly. :) Happy to do whatever.
-Kees
--
Kees Cook
Pixel Security
Re: [PATCH] scsi: ibmvscsis: Ensure partition name is properly NUL terminated
On Tue, Sep 11, 2018 at 11:15 AM, Laura Abbott wrote: > While reviewing another part of the code, Kees noticed that the > strncpy of the partition name might not always be NUL terminated. Switch > to using strlcpy which does this safely. > > Reported-by: Kees Cook > Signed-off-by: Laura Abbott > --- > I realized looking at this that I probably should have made > this and my previous patch a series given this has context depending on > the other patch. I can resend if the scsi maintainers want. > --- > drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c > b/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c > index 9305440a00a1..1217bf2a28db 100644 > --- a/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c > +++ b/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c > @@ -3477,7 +3477,7 @@ static int ibmvscsis_probe(struct vio_dev *vdev, > snprintf(vscsi->eye, sizeof(vscsi->eye), "VSCSI%s", vdev->name); > > vscsi->dds.unit_id = vdev->unit_address; > - strncpy(vscsi->dds.partition_name, partition_name, > + strlcpy(vscsi->dds.partition_name, partition_name, Please use strscpy() in favor of strlcpy(). -Kees > sizeof(vscsi->dds.partition_name)); > vscsi->dds.partition_num = partition_number; > > -- > 2.17.1 > -- Kees Cook Pixel Security
Re: [PATCHv2] scsi: ibmvscsis: Fix a stringop-overflow warning
On Tue, Sep 11, 2018 at 11:05 AM, Laura Abbott wrote: > > There's currently a warning about string overflow with strncat: > > drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c: In function 'ibmvscsis_probe': > drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c:3479:2: error: 'strncat' specified > bound 64 equals destination size [-Werror=stringop-overflow=] > strncat(vscsi->eye, vdev->name, MAX_EYE); > ^~~~ > > Switch to a single snprintf instead of a strcpy + strcat to handle this > cleanly. > > Signed-off-by: Laura Abbott Reviewed-by: Kees Cook -Kees > --- > v2: Swtich to using snprintf per suggestion of Kees > --- > drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c > b/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c > index fac377320158..9305440a00a1 100644 > --- a/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c > +++ b/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c > @@ -3474,8 +3474,7 @@ static int ibmvscsis_probe(struct vio_dev *vdev, > vscsi->dds.window[LOCAL].liobn, > vscsi->dds.window[REMOTE].liobn); > > - strcpy(vscsi->eye, "VSCSI "); > - strncat(vscsi->eye, vdev->name, MAX_EYE); > + snprintf(vscsi->eye, sizeof(vscsi->eye), "VSCSI%s", vdev->name); > > vscsi->dds.unit_id = vdev->unit_address; > strncpy(vscsi->dds.partition_name, partition_name, > -- > 2.17.1 > -- Kees Cook Pixel Security
Re: [PATCHv2] scsi: ibmvscsis: Fix a stringop-overflow warning
On Tue, Sep 11, 2018 at 11:24 AM, Kees Cook wrote: > On Tue, Sep 11, 2018 at 11:05 AM, Laura Abbott wrote: >> >> There's currently a warning about string overflow with strncat: >> >> drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c: In function 'ibmvscsis_probe': >> drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c:3479:2: error: 'strncat' specified >> bound 64 equals destination size [-Werror=stringop-overflow=] >> strncat(vscsi->eye, vdev->name, MAX_EYE); >> ^~~~ >> >> Switch to a single snprintf instead of a strcpy + strcat to handle this >> cleanly. >> >> Signed-off-by: Laura Abbott > > Reviewed-by: Kees Cook > > -Kees > >> --- >> v2: Swtich to using snprintf per suggestion of Kees >> --- >> drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c | 3 +-- >> 1 file changed, 1 insertion(+), 2 deletions(-) >> >> diff --git a/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c >> b/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c >> index fac377320158..9305440a00a1 100644 >> --- a/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c >> +++ b/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c >> @@ -3474,8 +3474,7 @@ static int ibmvscsis_probe(struct vio_dev *vdev, >> vscsi->dds.window[LOCAL].liobn, >> vscsi->dds.window[REMOTE].liobn); >> >> - strcpy(vscsi->eye, "VSCSI "); >> - strncat(vscsi->eye, vdev->name, MAX_EYE); >> + snprintf(vscsi->eye, sizeof(vscsi->eye), "VSCSI%s", vdev->name); Eek. No, I take it back: Nick Desaulniers has better eyes than me and pointed out that there should be a space after VSCSI. -Kees >> >> vscsi->dds.unit_id = vdev->unit_address; >> strncpy(vscsi->dds.partition_name, partition_name, >> -- >> 2.17.1 >> > > > > -- > Kees Cook > Pixel Security -- Kees Cook Pixel Security
Re: [PATCHv2 2/2] scsi: ibmvscsis: Ensure partition name is properly NUL terminated
On Tue, Sep 11, 2018 at 12:22 PM, Laura Abbott wrote: > > While reviewing another part of the code, Kees noticed that the > strncpy of the partition name might not always be NUL terminated. Switch > to using strscpy which does this safely. > > Reported-by: Kees Cook > Signed-off-by: Laura Abbott Reviewed-by: Kees Cook -Kees > --- > v2: Switch to strscpy instead of just strlcpy > --- > drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c > b/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c > index b3a029ad07cd..f42a619198c4 100644 > --- a/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c > +++ b/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c > @@ -3477,7 +3477,7 @@ static int ibmvscsis_probe(struct vio_dev *vdev, > snprintf(vscsi->eye, sizeof(vscsi->eye), "VSCSI %s", vdev->name); > > vscsi->dds.unit_id = vdev->unit_address; > - strncpy(vscsi->dds.partition_name, partition_name, > + strscpy(vscsi->dds.partition_name, partition_name, > sizeof(vscsi->dds.partition_name)); > vscsi->dds.partition_num = partition_number; > > -- > 2.17.1 > -- Kees Cook Pixel Security
Re: [PATCH] scsi/aic94xx/aic94xx_hwi.c: Use dma_pool_zalloc
On Wed, Oct 31, 2018 at 9:19 PM, Souptick Joarder wrote:
> Replaced dma_pool_alloc + memset with dma_pool_zalloc
>
> Signed-off-by: Brajeswar Ghosh
> Signed-off-by: Souptick Joarder
Reviewed-by: Kees Cook
-Kees
> ---
> drivers/scsi/aic94xx/aic94xx_hwi.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/scsi/aic94xx/aic94xx_hwi.c
> b/drivers/scsi/aic94xx/aic94xx_hwi.c
> index 3b8ad55e59de..2bc7615193bd 100644
> --- a/drivers/scsi/aic94xx/aic94xx_hwi.c
> +++ b/drivers/scsi/aic94xx/aic94xx_hwi.c
> @@ -1057,14 +1057,13 @@ static struct asd_ascb *asd_ascb_alloc(struct
> asd_ha_struct *asd_ha,
>
> if (ascb) {
> ascb->dma_scb.size = sizeof(struct scb);
> - ascb->dma_scb.vaddr = dma_pool_alloc(asd_ha->scb_pool,
> + ascb->dma_scb.vaddr = dma_pool_zalloc(asd_ha->scb_pool,
> gfp_flags,
>
> &ascb->dma_scb.dma_handle);
> if (!ascb->dma_scb.vaddr) {
> kmem_cache_free(asd_ascb_cache, ascb);
> return NULL;
> }
> - memset(ascb->dma_scb.vaddr, 0, sizeof(struct scb));
> asd_init_ascb(asd_ha, ascb);
>
> spin_lock_irqsave(&seq->tc_index_lock, flags);
> --
> 2.17.1
>
--
Kees Cook
Re: [PATCH RFC 00/15] Zero ****s, hugload of hugs <3
On Fri, Nov 30, 2018 at 11:27 AM Jarkko Sakkinen wrote: > > In order to comply with the CoC, replace with a hug. Heh. I support the replacement of the stronger language, but I find "hug", "hugged", and "hugging" to be very weird replacements. Can we bikeshed this to "heck", "hecked", and "hecking" (or "heckin" to follow true Doggo meme style). "This API is hugged" doesn't make any sense to me. "This API is hecked" is better, or at least funnier (to me). "Hug this interface" similarly makes no sense, but "Heck this interface" seems better. "Don't touch my hecking code", "What the heck were they thinking?" etc... "hug" is odd. Better yet, since it's only 17 files, how about doing context-specific changes? "This API is terrible", "Hateful interface", "Don't touch my freakin' code", "What in the world were they thinking?" etc? -Kees > > Jarkko Sakkinen (15): > MIPS: replace with a hug > Documentation: replace with a hug > drm/nouveau: replace with a hug > m68k: replace with a hug > parisc: replace with a hug > cpufreq: replace with a hug > ide: replace with a hug > media: replace with a hug > mtd: replace with a hug > net/sunhme: replace with a hug > scsi: replace with a hug > inotify: replace with a hug > irq: replace with a hug > lib: replace with a hug > net: replace with a hug > > Documentation/kernel-hacking/locking.rst | 2 +- > arch/m68k/include/asm/sun3ints.h | 2 +- > arch/mips/pci/ops-bridge.c| 24 +-- > arch/mips/sgi-ip22/ip22-setup.c | 2 +- > arch/parisc/kernel/sys_parisc.c | 2 +- > drivers/cpufreq/powernow-k7.c | 2 +- > .../gpu/drm/nouveau/nvkm/subdev/bios/init.c | 2 +- > .../nouveau/nvkm/subdev/pmu/fuc/macros.fuc| 2 +- > drivers/ide/cmd640.c | 2 +- > drivers/media/i2c/bt819.c | 8 --- > drivers/mtd/mtd_blkdevs.c | 2 +- > drivers/net/ethernet/sun/sunhme.c | 4 ++-- > drivers/scsi/qlogicpti.h | 2 +- > fs/notify/inotify/inotify_user.c | 2 +- > kernel/irq/timings.c | 2 +- > lib/vsprintf.c| 2 +- > net/core/skbuff.c | 2 +- > 17 files changed, 33 insertions(+), 31 deletions(-) > > -- > 2.19.1 > -- Kees Cook
[PATCH v2] scsi/bfa: use designated initializers
Prepare to mark sensitive kernel structures for randomization by making
sure they're using designated initializers. These were identified during
allyesconfig builds of x86, arm, and arm64, with most initializer fixes
extracted from grsecurity.
Signed-off-by: Kees Cook
---
This has been updated now that struct bfa_fcs_mod_s was dropped.
---
drivers/scsi/bfa/bfa_fcs_lport.c | 29 -
drivers/scsi/bfa/bfa_modules.h | 12 ++--
2 files changed, 26 insertions(+), 15 deletions(-)
diff --git a/drivers/scsi/bfa/bfa_fcs_lport.c b/drivers/scsi/bfa/bfa_fcs_lport.c
index 4ddda72f60e6..64860c730ec9 100644
--- a/drivers/scsi/bfa/bfa_fcs_lport.c
+++ b/drivers/scsi/bfa/bfa_fcs_lport.c
@@ -90,15 +90,26 @@ static struct {
void(*offline) (struct bfa_fcs_lport_s *port);
} __port_action[] = {
{
- bfa_fcs_lport_unknown_init, bfa_fcs_lport_unknown_online,
- bfa_fcs_lport_unknown_offline}, {
- bfa_fcs_lport_fab_init, bfa_fcs_lport_fab_online,
- bfa_fcs_lport_fab_offline}, {
- bfa_fcs_lport_n2n_init, bfa_fcs_lport_n2n_online,
- bfa_fcs_lport_n2n_offline}, {
- bfa_fcs_lport_loop_init, bfa_fcs_lport_loop_online,
- bfa_fcs_lport_loop_offline},
- };
+ .init = bfa_fcs_lport_unknown_init,
+ .online = bfa_fcs_lport_unknown_online,
+ .offline = bfa_fcs_lport_unknown_offline
+ },
+ {
+ .init = bfa_fcs_lport_fab_init,
+ .online = bfa_fcs_lport_fab_online,
+ .offline = bfa_fcs_lport_fab_offline
+ },
+ {
+ .init = bfa_fcs_lport_n2n_init,
+ .online = bfa_fcs_lport_n2n_online,
+ .offline = bfa_fcs_lport_n2n_offline
+ },
+ {
+ .init = bfa_fcs_lport_loop_init,
+ .online = bfa_fcs_lport_loop_online,
+ .offline = bfa_fcs_lport_loop_offline
+ },
+};
/*
* fcs_port_sm FCS logical port state machine
diff --git a/drivers/scsi/bfa/bfa_modules.h b/drivers/scsi/bfa/bfa_modules.h
index 53135f21fa0e..640621b4c3da 100644
--- a/drivers/scsi/bfa/bfa_modules.h
+++ b/drivers/scsi/bfa/bfa_modules.h
@@ -79,12 +79,12 @@ enum {
\
extern struct bfa_module_s hal_mod_ ## __mod; \
struct bfa_module_s hal_mod_ ## __mod = { \
- bfa_ ## __mod ## _meminfo, \
- bfa_ ## __mod ## _attach, \
- bfa_ ## __mod ## _detach, \
- bfa_ ## __mod ## _start,\
- bfa_ ## __mod ## _stop, \
- bfa_ ## __mod ## _iocdisable, \
+ .meminfo = bfa_ ## __mod ## _meminfo, \
+ .attach = bfa_ ## __mod ## _attach, \
+ .detach = bfa_ ## __mod ## _detach, \
+ .start = bfa_ ## __mod ## _start, \
+ .stop = bfa_ ## __mod ## _stop, \
+ .iocdisable = bfa_ ## __mod ## _iocdisable, \
}
#define BFA_CACHELINE_SZ (256)
--
2.7.4
--
Kees Cook
Pixel Security
[PATCH] scsi: qedi,qedf: Use designated initializers
Prepare to mark sensitive kernel structures for randomization by making
sure they're using designated initializers. These were identified during
allyesconfig builds of x86, arm, and arm64, with most initializer fixes
extracted from grsecurity.
For these cases, terminate the list with { }, which will be zero-filled,
instead of undesignated NULLs.
Signed-off-by: Kees Cook
---
drivers/scsi/qedf/qedf_debugfs.c | 2 +-
drivers/scsi/qedi/qedi_debugfs.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/scsi/qedf/qedf_debugfs.c b/drivers/scsi/qedf/qedf_debugfs.c
index cb08b625c594..00a1d6405ebe 100644
--- a/drivers/scsi/qedf/qedf_debugfs.c
+++ b/drivers/scsi/qedf/qedf_debugfs.c
@@ -449,7 +449,7 @@ const struct file_operations qedf_dbg_fops[] = {
qedf_dbg_fileops(qedf, clear_stats),
qedf_dbg_fileops_seq(qedf, offload_stats),
/* This must be last */
- { NULL, NULL },
+ { },
};
#else /* CONFIG_DEBUG_FS */
diff --git a/drivers/scsi/qedi/qedi_debugfs.c b/drivers/scsi/qedi/qedi_debugfs.c
index 59417199bf36..39d77818a677 100644
--- a/drivers/scsi/qedi/qedi_debugfs.c
+++ b/drivers/scsi/qedi/qedi_debugfs.c
@@ -240,5 +240,5 @@ const struct file_operations qedi_dbg_fops[] = {
qedi_dbg_fileops_seq(qedi, gbl_ctx),
qedi_dbg_fileops(qedi, do_not_recover),
qedi_dbg_fileops_seq(qedi, io_trace),
- { NULL, NULL },
+ { },
};
--
2.7.4
--
Kees Cook
Pixel Security
Re: [PATCH v2] scsi/bfa: use designated initializers
On Thu, Mar 30, 2017 at 1:18 AM, Christoph Hellwig wrote: > On Wed, Mar 29, 2017 at 01:55:09PM -0700, Kees Cook wrote: >> Prepare to mark sensitive kernel structures for randomization by making >> sure they're using designated initializers. These were identified during >> allyesconfig builds of x86, arm, and arm64, with most initializer fixes >> extracted from grsecurity. >> >> Signed-off-by: Kees Cook >> --- >> This has been updated now that struct bfa_fcs_mod_s was dropped. > > Can you also add designated array initializers for __port_action while > you're at it? Hrm? This is what the patch already does. It adds them for both __port_action and struct bfa_module_s. -Kees -- Kees Cook Pixel Security
[PATCH v3] scsi/bfa: use designated initializers
Prepare to mark sensitive kernel structures for randomization by making
sure they're using designated initializers. This also initializes the
array members using the enum used to look up __port_action entries.
Signed-off-by: Kees Cook
---
v3:
- drop bfa_module_s changes, since that has been removed entirely; hch.
- init array with enum literals; hch.
---
drivers/scsi/bfa/bfa_fcs_lport.c | 31 +--
1 file changed, 21 insertions(+), 10 deletions(-)
diff --git a/drivers/scsi/bfa/bfa_fcs_lport.c b/drivers/scsi/bfa/bfa_fcs_lport.c
index 4ddda72f60e6..638c0a2857f7 100644
--- a/drivers/scsi/bfa/bfa_fcs_lport.c
+++ b/drivers/scsi/bfa/bfa_fcs_lport.c
@@ -89,16 +89,27 @@ static struct {
void(*online) (struct bfa_fcs_lport_s *port);
void(*offline) (struct bfa_fcs_lport_s *port);
} __port_action[] = {
- {
- bfa_fcs_lport_unknown_init, bfa_fcs_lport_unknown_online,
- bfa_fcs_lport_unknown_offline}, {
- bfa_fcs_lport_fab_init, bfa_fcs_lport_fab_online,
- bfa_fcs_lport_fab_offline}, {
- bfa_fcs_lport_n2n_init, bfa_fcs_lport_n2n_online,
- bfa_fcs_lport_n2n_offline}, {
- bfa_fcs_lport_loop_init, bfa_fcs_lport_loop_online,
- bfa_fcs_lport_loop_offline},
- };
+ [BFA_FCS_FABRIC_UNKNOWN] = {
+ .init = bfa_fcs_lport_unknown_init,
+ .online = bfa_fcs_lport_unknown_online,
+ .offline = bfa_fcs_lport_unknown_offline
+ },
+ [BFA_FCS_FABRIC_SWITCHED] = {
+ .init = bfa_fcs_lport_fab_init,
+ .online = bfa_fcs_lport_fab_online,
+ .offline = bfa_fcs_lport_fab_offline
+ },
+ [BFA_FCS_FABRIC_N2N] = {
+ .init = bfa_fcs_lport_n2n_init,
+ .online = bfa_fcs_lport_n2n_online,
+ .offline = bfa_fcs_lport_n2n_offline
+ },
+ [BFA_FCS_FABRIC_LOOP] = {
+ .init = bfa_fcs_lport_loop_init,
+ .online = bfa_fcs_lport_loop_online,
+ .offline = bfa_fcs_lport_loop_offline
+ },
+};
/*
* fcs_port_sm FCS logical port state machine
--
2.7.4
--
Kees Cook
Pixel Security
Re: [PATCH 0/5] v2: block subsystem refcounter conversions
On Fri, Apr 21, 2017 at 3:55 AM, Reshetova, Elena wrote: > On Thu, Apr 20, 2017 at 11:33 AM, Eric Biggers wrote: >> Of course, having extra checks behind a debug option is fine. But they >> should >> not be part of the base feature; the base feature should just be mitigation >> of >> reference count *overflows*. It would be nice to do more, of course; but >> when >> the extra stuff prevents people from using refcount_t for performance >> reasons, >> it defeats the point of the feature in the first place. > > Sure, but as I said above, I think the smaller tricks and fixes won't be > convincing enough, > so their value is questionable. > >> I strongly encourage anyone who has been involved in refcount_t to experiment >> with removing a reference count decrement somewhere in their kernel, then >> trying >> to exploit it to gain code execution. If you don't know what you're trying >> to >> protect against, you will not know which defences work and which don't. > > Well, we had successful CVEs and even exploits on this in the past. > @Kees, do you store a list of them in the project? Here are two from last year: http://perception-point.io/2016/01/14/analysis-and-exploitation-of-a-linux-kernel-vulnerability-cve-2016-0728/ http://cyseclabs.com/page?n=02012016 I don't disagree with Eric on the threat model: the primary concern for reference count protection is the overflow condition. Detecting a prior use-after-free is certainly nice to have, but the more important case is the initial overflow. How about we introduce something like CONFIG_HAVE_ARCH_FAST_REFCOUNT_T for per-arch implementations and CONFIG_FAST_REFCOUNT_T that trades coverage for speed, and checks only the overflow condition. This gets us the critical coverage without the changes in performance. This is basically what PaX/grsecurity already did: there is a tiny change to the atomic inc functions to detect the wrap. -Kees -- Kees Cook Pixel Security
Re: [PATCH 0/5] v2: block subsystem refcounter conversions
On Fri, Apr 21, 2017 at 12:55 PM, Eric Biggers wrote: > Hi Elena, > > On Fri, Apr 21, 2017 at 10:55:29AM +, Reshetova, Elena wrote: >> > >> > At the very least, what is there now could probably be made about twice as >> > fast >> > by removing the checks that don't actually help mitigate refcount overflow >> > bugs, >> > specifically all the checks in refcount_dec(), and the part of >> > refcount_inc() >> > where it doesn't allow incrementing a 0 refcount. Hint: if a refcount is >> > 0, the >> > object has already been freed, so the attacker will have had the >> > opportunity to >> > allocate it with contents they control already. >> >> refcount_dec() is used very little through the code actually, it is more >> like an exception >> case since in order to use it one must really be sure that refcounter >> doesn't drop to zero. >> Removing the warn around it wouldn't likely affect much overall and thus it >> is better to >> stay to discourage people of API itself :) >> >> refcount_inc() is of course a different story, it is extensively used. I >> guess the perf issue >> on checking increment from zero might only come from WARNs being printed, >> but not really from an additional check here for zero since it is trivial >> and part of >> the needed loop anyway. So, I think only removing the >> WARNs might have any visible impact, but even this is probably not really >> that big. >> >> So, I think these changes won't really help adoption of interface if >> arguments against >> is performance. If we do have a performance issue, I think arch. specific >> implementation >> is likely the only way to considerably speed it up. > > I should have used refcount_dec_and_test() as the example, as the same applies > to both refcount_dec() and refcount_dec_and_test(). There is negligible > security benefit to have these refcount release operations checked vs. just > calling through to atomic_dec() and atomic_dec_and_test(). It's unfortunate, > but there is no known way to detect ahead of time (i.e. before exploitation) > if > there are too many refcount releases, only too many refcount acquires. > > The WARN is only executed if there is a bug, so naturally it's only a problem > if > the functions are to be inlined, creating bloat. The actual performance > problem > is the overhead associated with using comparisons and cmpxchg's to avoid > changing a refcount that is 0 or UINT_MAX. The more efficient approach would > be > to (a) drop the check for 0, and (b) don't require full operation to be > atomic, > but rather do something like "lock incl %[counter]; js ". Yes > it's not "atomic", and people have complained about this, but there is no > technical reason why it needs to be atomic. Attackers do *not* care whether > your exploit mitigation is theoretically "atomic" or not, they only care > whether > it works or not. And besides, it's not even "atomic_t" anymore, it's > "refcount_t". > >> > Of course, having extra checks behind a debug option is fine. But they >> > should >> > not be part of the base feature; the base feature should just be >> > mitigation of >> > reference count *overflows*. It would be nice to do more, of course; but >> > when >> > the extra stuff prevents people from using refcount_t for performance >> > reasons, >> > it defeats the point of the feature in the first place. >> >> Sure, but as I said above, I think the smaller tricks and fixes won't be >> convincing enough, >> so their value is questionable. > > This makes no sense, as the main point of the feature is supposed to be the > security improvement. As-is, the extra debugging stuff is actually preventing > the security improvement from being adopted, which is unfortunate. We've been trying to handle the conflicting desires of those wanting very precise refcounting implementation and gaining the security protections. Ultimately, the best way forward seemed to be to first land the precise refcounting implementation, and start conversion until we ran into concerns over performance. Now, since we're here, we can move forward with getting a fast implementation that provides the desired security protections without too greatly messing with the refcount API. -Kees -- Kees Cook Pixel Security
Re: [PATCH 0/5] v2: block subsystem refcounter conversions
On Fri, Apr 21, 2017 at 2:27 PM, James Bottomley wrote: > On Fri, 2017-04-21 at 13:22 -0700, Kees Cook wrote: >> On Fri, Apr 21, 2017 at 12:55 PM, Eric Biggers >> wrote: >> > > > Of course, having extra checks behind a debug option is fine. >> > > > But they should not be part of the base feature; the base >> > > > feature should just be mitigation of reference count >> > > > *overflows*. It would be nice to do more, of >> > > > course; but when the extra stuff prevents people from using >> > > > refcount_t for performance reasons, it defeats the point of the >> > > > feature in the first place. >> > > >> > > Sure, but as I said above, I think the smaller tricks and fixes >> > > won't be convincing enough, so their value is questionable. >> > >> > This makes no sense, as the main point of the feature is supposed >> > to be the security improvement. As-is, the extra debugging stuff >> > is actually preventing the security improvement from being adopted, >> > which is unfortunate. >> >> We've been trying to handle the conflicting desires of those wanting >> very precise refcounting implementation and gaining the security >> protections. Ultimately, the best way forward seemed to be to first >> land the precise refcounting implementation, and start conversion >> until we ran into concerns over performance. Now, since we're here, >> we can move forward with getting a fast implementation that provides >> the desired security protections without too greatly messing with the >> refcount API. > > But that's not what it really looks like. What it looks like is > someone came up with a new API and is now intent on forcing it through > the kernel in about 500 patches using security as the hammer. The intent is to split refcounting and statistical counters away from atomics, since they have distinct APIs. This will let us audit the remaining atomic uses much more easily. > If we were really concerned about security first, we'd have fixed the > atomic overflow problem in the atomics and then built the precise > refcounting on that and tried to persuade people of the merits. I agree, but this approach was NAKed by multiple atomics maintainers. > Why can't we still do this? It looks like the overflow protection will > add only a couple of cycles to the atomics. We can move the current > version to an unchecked variant which can be used either in truly > performance critical regions with no security implications or if > someone really needs the atomic to overflow. From my point of view it > would give us the 90% case (security) and stop this endless argument > over the fast paths. Subsystems which have already moved to refcount > would stay there and the rest get to evaluate a migration on the merits > of the operational utility. -Kees -- Kees Cook Pixel Security
[PATCH] scsi: qedf: Avoid reading past end of buffer
Using memcpy() from a string that is shorter than the length copied means
the destination buffer is being filled with arbitrary data from the kernel
rodata segment. Instead, use strncpy() which will fill the trailing bytes
with zeros.
This was found with the future CONFIG_FORTIFY_SOURCE feature.
Cc: Daniel Micay
Signed-off-by: Kees Cook
---
drivers/scsi/qedf/qedf_main.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/scsi/qedf/qedf_main.c b/drivers/scsi/qedf/qedf_main.c
index cceddd995a4b..a5c97342fd5d 100644
--- a/drivers/scsi/qedf/qedf_main.c
+++ b/drivers/scsi/qedf/qedf_main.c
@@ -2895,7 +2895,7 @@ static int __qedf_probe(struct pci_dev *pdev, int mode)
slowpath_params.drv_minor = QEDF_DRIVER_MINOR_VER;
slowpath_params.drv_rev = QEDF_DRIVER_REV_VER;
slowpath_params.drv_eng = QEDF_DRIVER_ENG_VER;
- memcpy(slowpath_params.name, "qedf", QED_DRV_VER_STR_SIZE);
+ strncpy(slowpath_params.name, "qedf", QED_DRV_VER_STR_SIZE);
rc = qed_ops->common->slowpath_start(qedf->cdev, &slowpath_params);
if (rc) {
QEDF_ERR(&(qedf->dbg_ctx), "Cannot start slowpath.\n");
--
2.7.4
--
Kees Cook
Pixel Security
Re: [PATCH] scsi: qedf: Avoid reading past end of buffer
On Fri, May 5, 2017 at 4:01 PM, Bart Van Assche
wrote:
> On Fri, 2017-05-05 at 15:42 -0700, Kees Cook wrote:
>> diff --git a/drivers/scsi/qedf/qedf_main.c b/drivers/scsi/qedf/qedf_main.c
>> index cceddd995a4b..a5c97342fd5d 100644
>> --- a/drivers/scsi/qedf/qedf_main.c
>> +++ b/drivers/scsi/qedf/qedf_main.c
>> @@ -2895,7 +2895,7 @@ static int __qedf_probe(struct pci_dev *pdev, int mode)
>> slowpath_params.drv_minor = QEDF_DRIVER_MINOR_VER;
>> slowpath_params.drv_rev = QEDF_DRIVER_REV_VER;
>> slowpath_params.drv_eng = QEDF_DRIVER_ENG_VER;
>> - memcpy(slowpath_params.name, "qedf", QED_DRV_VER_STR_SIZE);
>> + strncpy(slowpath_params.name, "qedf", QED_DRV_VER_STR_SIZE);
>> rc = qed_ops->common->slowpath_start(qedf->cdev, &slowpath_params);
>> if (rc) {
>> QEDF_ERR(&(qedf->dbg_ctx), "Cannot start slowpath.\n");
>
> Hello Kees,
>
> Although this patch looks fine to me, isn't strlcpy() preferred over
> strncpy()?
strlcpy doesn't zero-pad, so I think strncpy is preferred here,
otherwise we may risk leaving portions of the destination buffer
filled with uninitialized data, maybe leaking kernel memory contents.
-Kees
--
Kees Cook
Pixel Security
[PATCH] csiostor: Avoid content leaks and casts
When copying attributes, the len argument was padded out and the resulting
memcpy() would copy beyond the end of the source buffer. Avoid this,
and use size_t for val_len to avoid all the casts. Similarly, avoid source
buffer casts and use void *.
Additionally enforces val_len can be represented by u16 and that
the DMA buffer was not overflowed. Fixes the size of mfa, which is not
FC_FDMI_PORT_ATTR_MAXFRAMESIZE_LEN (but it will be padded up to 4). This
was noticed by the future CONFIG_FORTIFY_SOURCE checks.
Cc: Daniel Micay
Signed-off-by: Kees Cook
---
drivers/scsi/csiostor/csio_lnode.c | 43 +++---
1 file changed, 26 insertions(+), 17 deletions(-)
diff --git a/drivers/scsi/csiostor/csio_lnode.c
b/drivers/scsi/csiostor/csio_lnode.c
index c00b2ff72b55..be5ee2d37815 100644
--- a/drivers/scsi/csiostor/csio_lnode.c
+++ b/drivers/scsi/csiostor/csio_lnode.c
@@ -238,14 +238,23 @@ csio_osname(uint8_t *buf, size_t buf_len)
}
static inline void
-csio_append_attrib(uint8_t **ptr, uint16_t type, uint8_t *val, uint16_t len)
+csio_append_attrib(uint8_t **ptr, uint16_t type, void *val, size_t val_len)
{
+ uint16_t len;
struct fc_fdmi_attr_entry *ae = (struct fc_fdmi_attr_entry *)*ptr;
+
+ if (WARN_ON(val_len > U16_MAX))
+ return;
+
+ len = val_len;
+
ae->type = htons(type);
len += 4; /* includes attribute type and length */
len = (len + 3) & ~3; /* should be multiple of 4 bytes */
ae->len = htons(len);
- memcpy(ae->value, val, len);
+ memcpy(ae->value, val, val_len);
+ if (len > val_len)
+ memset(ae->value + val_len, 0, len - val_len);
*ptr += len;
}
@@ -335,7 +344,7 @@ csio_ln_fdmi_rhba_cbfn(struct csio_hw *hw, struct
csio_ioreq *fdmi_req)
numattrs++;
val = htonl(FC_PORTSPEED_1GBIT | FC_PORTSPEED_10GBIT);
csio_append_attrib(&pld, FC_FDMI_PORT_ATTR_SUPPORTEDSPEED,
- (uint8_t *)&val,
+ &val,
FC_FDMI_PORT_ATTR_SUPPORTEDSPEED_LEN);
numattrs++;
@@ -346,23 +355,22 @@ csio_ln_fdmi_rhba_cbfn(struct csio_hw *hw, struct
csio_ioreq *fdmi_req)
else
val = htonl(CSIO_HBA_PORTSPEED_UNKNOWN);
csio_append_attrib(&pld, FC_FDMI_PORT_ATTR_CURRENTPORTSPEED,
- (uint8_t *)&val,
- FC_FDMI_PORT_ATTR_CURRENTPORTSPEED_LEN);
+ &val, FC_FDMI_PORT_ATTR_CURRENTPORTSPEED_LEN);
numattrs++;
mfs = ln->ln_sparm.csp.sp_bb_data;
csio_append_attrib(&pld, FC_FDMI_PORT_ATTR_MAXFRAMESIZE,
- (uint8_t *)&mfs, FC_FDMI_PORT_ATTR_MAXFRAMESIZE_LEN);
+ &mfs, sizeof(mfs));
numattrs++;
strcpy(buf, "csiostor");
csio_append_attrib(&pld, FC_FDMI_PORT_ATTR_OSDEVICENAME, buf,
- (uint16_t)strlen(buf));
+ strlen(buf));
numattrs++;
if (!csio_hostname(buf, sizeof(buf))) {
csio_append_attrib(&pld, FC_FDMI_PORT_ATTR_HOSTNAME,
- buf, (uint16_t)strlen(buf));
+ buf, strlen(buf));
numattrs++;
}
attrib_blk->numattrs = htonl(numattrs);
@@ -444,33 +452,32 @@ csio_ln_fdmi_dprt_cbfn(struct csio_hw *hw, struct
csio_ioreq *fdmi_req)
strcpy(buf, "Chelsio Communications");
csio_append_attrib(&pld, FC_FDMI_HBA_ATTR_MANUFACTURER, buf,
- (uint16_t)strlen(buf));
+ strlen(buf));
numattrs++;
csio_append_attrib(&pld, FC_FDMI_HBA_ATTR_SERIALNUMBER,
- hw->vpd.sn, (uint16_t)sizeof(hw->vpd.sn));
+ hw->vpd.sn, sizeof(hw->vpd.sn));
numattrs++;
csio_append_attrib(&pld, FC_FDMI_HBA_ATTR_MODEL, hw->vpd.id,
- (uint16_t)sizeof(hw->vpd.id));
+ sizeof(hw->vpd.id));
numattrs++;
csio_append_attrib(&pld, FC_FDMI_HBA_ATTR_MODELDESCRIPTION,
- hw->model_desc, (uint16_t)strlen(hw->model_desc));
+ hw->model_desc, strlen(hw->model_desc));
numattrs++;
csio_append_attrib(&pld, FC_FDMI_HBA_ATTR_HARDWAREVERSION,
- hw->hw_ver, (uint16_t)sizeof(hw->hw_ver));
+ hw->hw_ver, sizeof(hw->hw_ver));
numattrs++;
csio_append_attrib(&pld, FC_FDMI_HBA_ATTR_FIRMWAREVERSION,
- hw->fwrev_str, (uint16_t)strlen(hw->fwrev_str));
+ hw->fwrev_str, strlen(hw->fwrev_str));
numattrs++;
Re: [PATCH] csiostor: Avoid content leaks and casts
On Mon, May 22, 2017 at 8:05 AM, Varun Prakash wrote: > On Tue, May 09, 2017 at 03:34:44PM -0700, Kees Cook wrote: >> When copying attributes, the len argument was padded out and the resulting >> memcpy() would copy beyond the end of the source buffer. Avoid this, >> and use size_t for val_len to avoid all the casts. Similarly, avoid source >> buffer casts and use void *. >> >> Additionally enforces val_len can be represented by u16 and that >> the DMA buffer was not overflowed. Fixes the size of mfa, which is not >> FC_FDMI_PORT_ATTR_MAXFRAMESIZE_LEN (but it will be padded up to 4). This >> was noticed by the future CONFIG_FORTIFY_SOURCE checks. >> >> Cc: Daniel Micay >> Signed-off-by: Kees Cook >> --- >> drivers/scsi/csiostor/csio_lnode.c | 43 >> +++--- >> 1 file changed, 26 insertions(+), 17 deletions(-) >> >> diff --git a/drivers/scsi/csiostor/csio_lnode.c >> b/drivers/scsi/csiostor/csio_lnode.c >> index c00b2ff72b55..be5ee2d37815 100644 >> --- a/drivers/scsi/csiostor/csio_lnode.c >> +++ b/drivers/scsi/csiostor/csio_lnode.c >> @@ -238,14 +238,23 @@ csio_osname(uint8_t *buf, size_t buf_len) >> } >> > > >> >> csio_append_attrib(&pld, FC_FDMI_HBA_ATTR_MAXCTPAYLOAD, >> -(uint8_t *)&maxpayload, >> -FC_FDMI_HBA_ATTR_MAXCTPAYLOAD_LEN); >> +&maxpayload, FC_FDMI_HBA_ATTR_MAXCTPAYLOAD_LEN); >> len = (uint32_t)(pld - (uint8_t *)cmd); >> numattrs++; >> attrib_blk->numattrs = htonl(numattrs); >> @@ -1794,6 +1801,8 @@ csio_ln_mgmt_submit_req(struct csio_ioreq *io_req, >> struct csio_mgmtm *mgmtm = csio_hw_to_mgmtm(hw); >> int rv; >> >> + BUG_ON(pld_len > pld->len); >> + > > I think WARN_ON() is better than BUG_ON() in this case > > if (WARN_ON(pld_len > pld->len)) > return -EINVAL; > >> io_req->io_cbfn = io_cbfn; /* Upper layer callback handler */ >> io_req->fw_handle = (uintptr_t) (io_req); >> io_req->eq_idx = mgmtm->eq_idx; I chose BUG_ON here because the damage has already been done. If this assertion is hit, the heap buffers have already been overrun. This isn't a state we should only warn about... -Kees -- Kees Cook Pixel Security
Re: usercopy whitelist woe in scsi_sense_cache
On Wed, Apr 4, 2018 at 12:07 PM, Oleksandr Natalenko
wrote:
> With v4.16 I get the following dump while using smartctl:
> [...]
> [ 261.262135] Bad or missing usercopy whitelist? Kernel memory exposure
> attempt detected from SLUB object 'scsi_sense_cache' (offset 94, size 22)!
> [...]
> [ 261.345976] Call Trace:
> [ 261.350620] __check_object_size+0x130/0x1a0
> [ 261.355775] sg_io+0x269/0x3f0
> [ 261.360729] ? path_lookupat+0xaa/0x1f0
> [ 261.364027] ? current_time+0x18/0x70
> [ 261.366684] scsi_cmd_ioctl+0x257/0x410
> [ 261.369871] ? xfs_bmapi_read+0x1c3/0x340 [xfs]
> [ 261.372231] sd_ioctl+0xbf/0x1a0 [sd_mod]
> [ 261.375456] blkdev_ioctl+0x8ca/0x990
> [ 261.381156] ? read_null+0x10/0x10
> [ 261.384984] block_ioctl+0x39/0x40
> [ 261.388739] do_vfs_ioctl+0xa4/0x630
> [ 261.392624] ? vfs_write+0x164/0x1a0
> [ 261.396658] SyS_ioctl+0x74/0x80
> [ 261.399563] do_syscall_64+0x74/0x190
> [ 261.402685] entry_SYSCALL_64_after_hwframe+0x3d/0xa2
This is:
sg_io+0x269/0x3f0:
blk_complete_sghdr_rq at block/scsi_ioctl.c:280
(inlined by) sg_io at block/scsi_ioctl.c:376
which is:
if (req->sense_len && hdr->sbp) {
int len = min((unsigned int) hdr->mx_sb_len, req->sense_len);
if (!copy_to_user(hdr->sbp, req->sense, len))
hdr->sb_len_wr = len;
else
ret = -EFAULT;
}
> [...]
> I can easily reproduce it with a qemu VM and 2 virtual SCSI disks by calling
> smartctl in a loop and doing some usual background I/O. The warning is
> triggered within 3 minutes or so (not instantly).
>
> Initially, it was produced on my server after a kernel update (because disks
> are monitored with smartctl via Zabbix).
>
> Looks like the thing was introduced with
> 0afe76e88c57d91ef5697720aed380a339e3df70.
>
> Any idea how to deal with this please? If needed, I can provide any additional
> info, and also I'm happy/ready to test any proposed patches.
Interesting, and a little confusing. So, what's strange here is that
the scsi_sense_cache already has a full whitelist:
kmem_cache_create_usercopy("scsi_sense_cache",
SCSI_SENSE_BUFFERSIZE, 0, SLAB_HWCACHE_ALIGN,
0, SCSI_SENSE_BUFFERSIZE, NULL);
Arg 2 is the buffer size, arg 5 is the whitelist offset (0), and the
whitelist size (same as arg2). In other words, the entire buffer
should be whitelisted.
include/scsi/scsi_cmnd.h says:
#define SCSI_SENSE_BUFFERSIZE 96
That means scsi_sense_cache should be 96 bytes in size? But a 22 byte
read starting at offset 94 happened? That seems like a 20 byte read
beyond the end of the SLUB object? Though if it were reading past the
actual end of the object, I'd expect the hardened usercopy BUG (rather
than the WARN) to kick in. Ah, it looks like
/sys/kernel/slab/scsi_sense_cache/slab_size shows this to be 128 bytes
of actual allocation, so the 20 bytes doesn't strictly overlap another
object (hence no BUG):
/sys/kernel/slab/scsi_sense_cache# grep . object_size usersize slab_size
object_size:96
usersize:96
slab_size:128
Ah, right, due to SLAB_HWCACHE_ALIGN, the allocation is rounded up to
the next cache line size, so there's 32 bytes of padding to reach 128.
James or Martin, is this over-read "expected" behavior? i.e. does the
sense cache buffer usage ever pull the ugly trick of silently
expanding its allocation into the space the slab allocator has given
it? If not, this looks like a real bug.
What I don't see is how req->sense is _not_ at offset 0 in the
scsi_sense_cache object...
-Kees
--
Kees Cook
Pixel Security
Re: usercopy whitelist woe in scsi_sense_cache
On Wed, Apr 4, 2018 at 12:07 PM, Oleksandr Natalenko wrote: > [ 261.262135] Bad or missing usercopy whitelist? Kernel memory exposure > attempt detected from SLUB object 'scsi_sense_cache' (offset 94, size 22)! > I can easily reproduce it with a qemu VM and 2 virtual SCSI disks by calling > smartctl in a loop and doing some usual background I/O. The warning is > triggered within 3 minutes or so (not instantly). Also: Can you send me your .config? What SCSI drivers are you using in the VM and on the real server? Are you able to see what ioctl()s smartctl is issuing? I'll try to reproduce this on my end... -Kees -- Kees Cook Pixel Security
Re: usercopy whitelist woe in scsi_sense_cache
On Wed, Apr 4, 2018 at 1:49 PM, Oleksandr Natalenko wrote: > Hi. > > On středa 4. dubna 2018 22:21:53 CEST Kees Cook wrote: >> >> ... >> That means scsi_sense_cache should be 96 bytes in size? But a 22 byte >> read starting at offset 94 happened? That seems like a 20 byte read >> beyond the end of the SLUB object? Though if it were reading past the >> actual end of the object, I'd expect the hardened usercopy BUG (rather >> than the WARN) to kick in. Ah, it looks like >> /sys/kernel/slab/scsi_sense_cache/slab_size shows this to be 128 bytes >> of actual allocation, so the 20 bytes doesn't strictly overlap another >> object (hence no BUG): >> ... > > > Actually, I can trigger a BUG too: > > [ 129.259213] usercopy: Kernel memory exposure attempt detected from SLUB > object 'scsi_sense_cache' (offset 119, size 22)! Wow, yeah, that's totally outside the slub object_size. How did you trigger this? Just luck or something specific? > [ 129.265167] [ cut here ] > [ 129.267579] kernel BUG at mm/usercopy.c:100! > > And also offset can be different, as you may see: > > [ 55.993224] Bad or missing usercopy whitelist? Kernel memory exposure > attempt detected from SLUB object 'scsi_sense_cache' (offset 76, size 22)! > [ 55.998678] WARNING: CPU: 0 PID: 1305 at mm/usercopy.c:81 usercopy_warn > +0x7e/0xa0 > > It looks like only the size stays the same. I'd really like to understand how the buffer position can be changing... I'd expect that to break all kinds of things (i.e. free()ing the slab later would break too...) >> Can you send me your .config? What SCSI drivers are you using in the >> VM and on the real server? > > This is an Arch kernel with a config available here [1]. > > For both server and VM "lspci -vv" shows "ahci" in use. Is this what you are > asking for? I think so, yeah. >> Are you able to see what ioctl()s smartctl is issuing? I'll try to >> reproduce this on my end... > > As per [2], strace shows "SG_IO" requests. Is this detailed enough? That's useful, yeah. I'll try Douglas's suggestion of "smartctl -r scsiioctl,3 ..." too. > Thanks for looking into it. Thanks for the report! I hope someone more familiar with sg_io() can help explain the changing buffer offset... :P -Kees -- Kees Cook Pixel Security
Re: usercopy whitelist woe in scsi_sense_cache
On Thu, Apr 5, 2018 at 2:56 AM, Oleksandr Natalenko wrote: > Hi. > > 04.04.2018 23:25, Kees Cook wrote: >> >> Thanks for the report! I hope someone more familiar with sg_io() can >> help explain the changing buffer offset... :P > > > Also, FYI, I kept the server running with smartctl periodically invoked, and > it was still triggering BUGs, however, I consider them to be more or less > harmless until the server got stuck with high I/O wait this morning after > next smartctl invocation. So, it isn't harmless, it seems… > > It could be unrelated, of course, since the journal didn't give me any hint > (or a stack trace) on what happened, thus I'll monitor how things behave > without smartctl too. I had a VM running over night with: [1] Running while :; do smartctl -a /dev/sda > /dev/null; done & [2]- Running while :; do ls --color=auto -lR / > /dev/null 2> /dev/null; done & [3]+ Running while :; do sleep $(( $RANDOM % 100 )); sync; echo 3 > /proc/sys/vm/drop_caches; done & and I haven't seen the issue. :( FWIW, I'm using the ahci qemu driver: -drive file=disk-image.raw,if=none,id=drive0,format=raw \ -device ahci,id=bus0 \ -device ide-drive,bus=bus0.0,drive=drive0 Does this match your qemu instance? -Kees -- Kees Cook Pixel Security
Re: usercopy whitelist woe in scsi_sense_cache
[forcing non-HTML and resending...] On Thu, Apr 5, 2018 at 7:33 AM, Oleksandr Natalenko wrote: > > 05.04.2018 16:32, Oleksandr Natalenko wrote: >> >> "-hda sda.img -hdb sda.img" > > > "-hda sda.img -hdb sdb.img", of course, I don't pass the same disk twice Okay. My qemu gets mad about that and wants the format=raw argument, so I'm using: -drive file=sda.img,format=raw \ -drive file=sdb.img,format=raw \ How are you running your smartctl? I'm doing this now: [1] Running while :; do ( smartctl -a /dev/sda; smartctl -a /dev/sdb ) > /dev/null; done & I assume I'm missing something from your .config, but since I don't boot with an initramfs, I had to tweak it a bit. I'll try again... -Kees -- Kees Cook Pixel Security
Re: usercopy whitelist woe in scsi_sense_cache
On Sun, Apr 8, 2018 at 12:07 PM, Oleksandr Natalenko wrote: > So far, I wasn't able to trigger this with mq-deadline (or without blk-mq). > Maybe, this has something to do with blk-mq+BFQ re-queuing, or it's just me > not being persistent enough. Ah, this detail I didn't have. I've changed my environment to build with: CONFIG_BLK_MQ_PCI=y CONFIG_BLK_MQ_VIRTIO=y CONFIG_IOSCHED_BFQ=y boot with scsi_mod.use_blk_mq=1 and select BFQ in the scheduler: # cat /sys/block/sd?/queue/scheduler mq-deadline kyber [bfq] none mq-deadline kyber [bfq] none Even with this, I'm not seeing anything yet... > It looks like this code path was re-written completely with 17cb960f29c2, but > it went merged for the upcoming v4.17 only, and thus I haven't tried it yet. > > Kees took a brief look at it already: [1]. This is what smartctl does [2] > (just a usual strace capture when the bug is not triggered). > > Christoph, do you have some idea on why this can happen? > > Thanks. > > Regards, > Oleksandr > > [1] https://marc.info/?l=linux-scsi&m=152287333013845&w=2 > [2] https://gist.github.com/pfactum/6f58f8891468aeba1ab2cc9f45668735 The thing I can't figure out is how req->sense is slipping forward in (and even beyond!) the allocation. -Kees -- Kees Cook Pixel Security
Re: usercopy whitelist woe in scsi_sense_cache
On Mon, Apr 9, 2018 at 12:02 PM, Oleksandr Natalenko wrote: > > Hi. > > (fancy details for linux-block and BFQ people go below) > > 09.04.2018 20:32, Kees Cook wrote: >> >> Ah, this detail I didn't have. I've changed my environment to >> >> build with: >> >> CONFIG_BLK_MQ_PCI=y >> CONFIG_BLK_MQ_VIRTIO=y >> CONFIG_IOSCHED_BFQ=y >> >> boot with scsi_mod.use_blk_mq=1 >> >> and select BFQ in the scheduler: >> >> # cat /sys/block/sd?/queue/scheduler >> mq-deadline kyber [bfq] none >> mq-deadline kyber [bfq] none >> >> Even with this, I'm not seeing anything yet... > > > Thanks for looking into it anyway. I was experimenting today a little bit, > and for me it looks like setting queue_depth and nr_requests to minimal > values speeds up the reproducing. Could you please try it too? Something like: > > echo 1 | tee /sys/block/sd*/queue/nr_requests I can't get this below "4". > echo 1 | tee /sys/block/sd*/device/queue_depth I've got this now too. > Also, now I have a more solid proof that this is related to I/O scheduling. > > I was hammering my VM, and after a couple of usercopy warnings/bugs I can > reliably trigger I/O hang. I was able to obtain the stack traces of tasks in > D state. Listing them here below. dmcrypt_write: Ah! dm-crypt too. I'll see if I can get that added easily to my tests. > === > [ 1615.409622] dmcrypt_write D0 236 2 0x8000 > [ 1615.413422] Call Trace: > [ 1615.415428] ? __schedule+0x336/0xf40 > [ 1615.417824] ? blk_mq_sched_dispatch_requests+0x117/0x190 > [ 1615.421423] ? __sbitmap_get_word+0x2a/0x80 > [ 1615.424202] schedule+0x32/0xc0 > [ 1615.426521] io_schedule+0x12/0x40 > [ 1615.432414] blk_mq_get_tag+0x181/0x2a0 > [ 1615.434881] ? elv_merge+0x79/0xe0 > [ 1615.437447] ? wait_woken+0x80/0x80 > [ 1615.439553] blk_mq_get_request+0xf9/0x400 > [ 1615.444653] blk_mq_make_request+0x10b/0x640 > [ 1615.448025] generic_make_request+0x124/0x2d0 > [ 1615.450716] ? raid10_unplug+0xfb/0x180 [raid10] > [ 1615.454069] raid10_unplug+0xfb/0x180 [raid10] > [ 1615.456729] blk_flush_plug_list+0xc1/0x250 > [ 1615.460276] blk_finish_plug+0x27/0x40 > [ 1615.464103] dmcrypt_write+0x233/0x240 [dm_crypt] > [ 1615.467443] ? wake_up_process+0x20/0x20 > [ 1615.470845] ? crypt_iv_essiv_dtr+0x60/0x60 [dm_crypt] > [ 1615.475272] ? kthread+0x113/0x130 > [ 1615.477652] kthread+0x113/0x130 > [ 1615.480567] ? kthread_create_on_node+0x70/0x70 > [ 1615.483268] ret_from_fork+0x35/0x40 > === > > One of XFS threads, too: And XFS! You love your corner cases. ;) > > === > [ 1616.133298] xfsaild/dm-7D0 316 2 0x8000 > [ 1616.136679] Call Trace: > [ 1616.138845] ? __schedule+0x336/0xf40 > [ 1616.141581] ? preempt_count_add+0x68/0xa0 > [ 1616.147214] ? _raw_spin_unlock+0x16/0x30 > [ 1616.149813] ? _raw_spin_unlock_irqrestore+0x20/0x40 > [ 1616.152478] ? try_to_del_timer_sync+0x4d/0x80 > [ 1616.154734] schedule+0x32/0xc0 > [ 1616.156579] _xfs_log_force+0x146/0x290 [xfs] > [ 1616.159322] ? wake_up_process+0x20/0x20 > [ 1616.162175] xfsaild+0x1a9/0x820 [xfs] > [ 1616.164695] ? xfs_trans_ail_cursor_first+0x80/0x80 [xfs] > [ 1616.167567] ? kthread+0x113/0x130 > [ 1616.169722] kthread+0x113/0x130 > [ 1616.171908] ? kthread_create_on_node+0x70/0x70 > [ 1616.174073] ? do_syscall_64+0x74/0x190 > [ 1616.179008] ? SyS_exit_group+0x10/0x10 > [ 1616.182306] ret_from_fork+0x35/0x40 > === > > journald is another victim: > > === > [ 1616.184343] systemd-journal D0 354 1 0x0104 > [ 1616.187282] Call Trace: > [ 1616.189464] ? __schedule+0x336/0xf40 > [ 1616.191781] ? call_function_single_interrupt+0xa/0x20 > [ 1616.194788] ? _raw_spin_lock_irqsave+0x25/0x50 > [ 1616.197592] schedule+0x32/0xc0 > [ 1616.200171] schedule_timeout+0x202/0x470 > [ 1616.202851] ? preempt_count_add+0x49/0xa0 > [ 1616.206227] wait_for_common+0xbb/0x180 > [ 1616.209877] ? wake_up_process+0x20/0x20 > [ 1616.212511] do_coredump+0x335/0xea0 > [ 1616.214861] ? schedule+0x3c/0xc0 > [ 1616.216775] ? futex_wait_queue_me+0xbb/0x110 > [ 1616.218894] ? _raw_spin_unlock+0x16/0x30 > [ 1616.220868] ? futex_wait+0x143/0x240 > [ 1616.223450] get_signal+0x250/0x5c0 > [ 1616.225965] do_signal+0x36/0x610 > [ 1616.228246] ? __seccomp_filter+0x3b/0x260 > [ 1616.231000] ? __check_object_size+0x9f/0x1a0 > [ 1616.233716] exit_to_usermode_loop+0x85/0xa0 > [ 1616.238413] do_syscall_64+0x18b/0x190 > [ 1616.240798] entry_SYSCALL_64_after_hwframe+0x3d/0xa2 > [ 1616.244401] RIP: 0033:0x7f78fc53e45d > [ 1616.246690] RSP: 002b:7ffd40330d20 EFLAGS: 024
Re: usercopy whitelist woe in scsi_sense_cache
On Mon, Apr 9, 2018 at 1:30 PM, Kees Cook wrote: > Ah! dm-crypt too. I'll see if I can get that added easily to my tests. Quick update: I added dm-crypt (with XFS on top) and it hung my system almost immediately. I got no warnings at all, though. -Kees -- Kees Cook Pixel Security
Re: usercopy whitelist woe in scsi_sense_cache
On Mon, Apr 9, 2018 at 11:35 PM, Oleksandr Natalenko wrote: > Did your system hang on smartctl hammering too? Have you got some stack > traces to compare with mine ones? Unfortunately I only had a single hang with no dumps. I haven't been able to reproduce it since. :( -Kees -- Kees Cook Pixel Security
Re: usercopy whitelist woe in scsi_sense_cache
On Tue, Apr 10, 2018 at 10:16 AM, Oleksandr Natalenko wrote: > Hi, Kees, Paolo et al. > > 10.04.2018 08:53, Kees Cook wrote: >> >> Unfortunately I only had a single hang with no dumps. I haven't been >> able to reproduce it since. :( > > > For your convenience I've prepared a VM that contains a reproducer. Awesome. :) > Under the /root folder there is a reproducer script (reproducer.sh). It does > trivial things like enabling sysrq, opening LUKS device, mounting a volume, > running a background I/O (this is an important part, actually, since I > wasn't able to trigger the issue without the background I/O) and, finally, > running the smartctl in a loop. If you are lucky, within a minute or two > you'll get the first warning followed shortly by subsequent bugs and I/O > stall (htop is pre-installed for your convenience too). Yup! [ 27.729498] Bad or missing usercopy whitelist? Kernel memory exposure attempt detected from SLUB object 'scsi_sense_cache' (offset 76, size 22)! I'll see about booting with my own kernels, etc, and try to narrow this down. :) -Kees -- Kees Cook Pixel Security
Re: usercopy whitelist woe in scsi_sense_cache
On Tue, Apr 10, 2018 at 8:13 PM, Kees Cook wrote: > I'll see about booting with my own kernels, etc, and try to narrow this down. > :) If I boot kernels I've built, I no longer hit the bug in this VM (though I'll keep trying). What compiler are you using? -Kees -- Kees Cook Pixel Security
Re: usercopy whitelist woe in scsi_sense_cache
On Wed, Apr 11, 2018 at 3:47 PM, Kees Cook wrote: > On Tue, Apr 10, 2018 at 8:13 PM, Kees Cook wrote: >> I'll see about booting with my own kernels, etc, and try to narrow this >> down. :) > > If I boot kernels I've built, I no longer hit the bug in this VM > (though I'll keep trying). What compiler are you using? Ignore that: I've reproduced it with my kernels now. I think I messed up the initramfs initially. But with an exact copy of your .config, booting under Arch grub with initramfs, I see it. I'll start removing variables now... :P -Kees -- Kees Cook Pixel Security
Re: usercopy whitelist woe in scsi_sense_cache
On Wed, Apr 11, 2018 at 5:03 PM, Kees Cook wrote:
> On Wed, Apr 11, 2018 at 3:47 PM, Kees Cook wrote:
>> On Tue, Apr 10, 2018 at 8:13 PM, Kees Cook wrote:
>>> I'll see about booting with my own kernels, etc, and try to narrow this
>>> down. :)
>>
>> If I boot kernels I've built, I no longer hit the bug in this VM
>> (though I'll keep trying). What compiler are you using?
>
> Ignore that: I've reproduced it with my kernels now. I think I messed
> up the initramfs initially. But with an exact copy of your .config,
> booting under Arch grub with initramfs, I see it. I'll start removing
> variables now... :P
My first bisect attempt gave me commit 5448aca41cd5 ("null_blk: wire
up timeouts"), which seems insane given that null_blk isn't even built
in the .config. I managed to get the testing automated now for a "git
bisect run ...", so I'm restarting again to hopefully get a better
answer. Normally the Oops happens in the first minute of running. I've
set the timeout to 10 minutes for a "good" run...
-Kees
--
Kees Cook
Pixel Security
Re: usercopy whitelist woe in scsi_sense_cache
On Thu, Apr 12, 2018 at 12:04 PM, Oleksandr Natalenko
wrote:
> Hi.
>
> On čtvrtek 12. dubna 2018 20:44:37 CEST Kees Cook wrote:
>> My first bisect attempt gave me commit 5448aca41cd5 ("null_blk: wire
>> up timeouts"), which seems insane given that null_blk isn't even built
>> in the .config. I managed to get the testing automated now for a "git
>> bisect run ...", so I'm restarting again to hopefully get a better
>> answer. Normally the Oops happens in the first minute of running. I've
>> set the timeout to 10 minutes for a "good" run...
>
> Apparently, you do this on Linus' tree, right? If so, I won't compile it here
> then.
Actually, I didn't test Linus's tree, but I can do that after the most
recent bisect finishes... I'm just trying to find where it went wrong
in 4.16.
> Thanks for taking care of this.
Thanks for building the reproducer! :)
-Kees
--
Kees Cook
Pixel Security
Re: usercopy whitelist woe in scsi_sense_cache
On Thu, Apr 12, 2018 at 3:01 PM, Kees Cook wrote:
> On Thu, Apr 12, 2018 at 12:04 PM, Oleksandr Natalenko
> wrote:
>> Hi.
>>
>> On čtvrtek 12. dubna 2018 20:44:37 CEST Kees Cook wrote:
>>> My first bisect attempt gave me commit 5448aca41cd5 ("null_blk: wire
>>> up timeouts"), which seems insane given that null_blk isn't even built
>>> in the .config. I managed to get the testing automated now for a "git
>>> bisect run ...", so I'm restarting again to hopefully get a better
>>> answer. Normally the Oops happens in the first minute of running. I've
>>> set the timeout to 10 minutes for a "good" run...
After fixing up some build issues in the middle of the 4.16 cycle, I
get an unhelpful bisect result of commit 0a4b6e2f80aa ("Merge branch
'for-4.16/block'". Instead of letting the test run longer, I'm going
to switch to doing several shorter test boots per kernel and see if
that helps. One more bisect coming...
>> Apparently, you do this on Linus' tree, right? If so, I won't compile it here
>> then.
>
> Actually, I didn't test Linus's tree, but I can do that after the most
> recent bisect finishes... I'm just trying to find where it went wrong
> in 4.16.
FWIW, I see an Oops under Linus's latest tree.
-Kees
--
Kees Cook
Pixel Security
Re: usercopy whitelist woe in scsi_sense_cache
On Thu, Apr 12, 2018 at 3:47 PM, Kees Cook wrote:
> After fixing up some build issues in the middle of the 4.16 cycle, I
> get an unhelpful bisect result of commit 0a4b6e2f80aa ("Merge branch
> 'for-4.16/block'"). Instead of letting the test run longer, I'm going
> to switch to doing several shorter test boots per kernel and see if
> that helps. One more bisect coming...
Okay, so I can confirm the bisect points at the _merge_ itself, not a
specific patch. I'm not sure how to proceed here. It looks like some
kind of interaction between separate trees? Jens, do you have
suggestions on how to track this down?
-Kees
--
Kees Cook
Pixel Security
Re: usercopy whitelist woe in scsi_sense_cache
On Thu, Apr 12, 2018 at 8:02 PM, Kees Cook wrote:
> On Thu, Apr 12, 2018 at 3:47 PM, Kees Cook wrote:
>> After fixing up some build issues in the middle of the 4.16 cycle, I
>> get an unhelpful bisect result of commit 0a4b6e2f80aa ("Merge branch
>> 'for-4.16/block'"). Instead of letting the test run longer, I'm going
>> to switch to doing several shorter test boots per kernel and see if
>> that helps. One more bisect coming...
>
> Okay, so I can confirm the bisect points at the _merge_ itself, not a
> specific patch. I'm not sure how to proceed here. It looks like some
> kind of interaction between separate trees? Jens, do you have
> suggestions on how to track this down?
Turning off HARDENED_USERCOPY and turning on KASAN, I see the same report:
[ 38.274106] BUG: KASAN: slab-out-of-bounds in _copy_to_user+0x42/0x60
[ 38.274841] Read of size 22 at addr 8800122b8c4b by task smartctl/1064
[ 38.275630]
[ 38.275818] CPU: 2 PID: 1064 Comm: smartctl Not tainted 4.17.0-rc1-ARCH+ #266
[ 38.276631] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009),
BIOS Ubuntu-1.8.2-1ubuntu1 04/01/2014
[ 38.277690] Call Trace:
[ 38.277988] dump_stack+0x71/0xab
[ 38.278397] ? _copy_to_user+0x42/0x60
[ 38.278833] print_address_description+0x6a/0x270
[ 38.279368] ? _copy_to_user+0x42/0x60
[ 38.279800] kasan_report+0x243/0x360
[ 38.280221] _copy_to_user+0x42/0x60
[ 38.280635] sg_io+0x459/0x660
...
Though we get slightly more details (some we already knew):
[ 38.301330] Allocated by task 329:
[ 38.301734] kmem_cache_alloc_node+0xca/0x220
[ 38.302239] scsi_mq_init_request+0x64/0x130 [scsi_mod]
[ 38.302821] blk_mq_alloc_rqs+0x2cf/0x370
[ 38.303265] blk_mq_sched_alloc_tags.isra.4+0x7d/0xb0
[ 38.303820] blk_mq_init_sched+0xf0/0x220
[ 38.304268] elevator_switch+0x17a/0x2c0
[ 38.304705] elv_iosched_store+0x173/0x220
[ 38.305171] queue_attr_store+0x72/0xb0
[ 38.305602] kernfs_fop_write+0x188/0x220
[ 38.306049] __vfs_write+0xb6/0x330
[ 38.306436] vfs_write+0xe9/0x240
[ 38.306804] ksys_write+0x98/0x110
[ 38.307181] do_syscall_64+0x6d/0x1d0
[ 38.307590] entry_SYSCALL_64_after_hwframe+0x44/0xa9
[ 38.308142]
[ 38.308316] Freed by task 0:
[ 38.308652] (stack is not available)
[ 38.309060]
[ 38.309243] The buggy address belongs to the object at 8800122b8c00
[ 38.309243] which belongs to the cache scsi_sense_cache of size 96
[ 38.310625] The buggy address is located 75 bytes inside of
[ 38.310625] 96-byte region [8800122b8c00, 8800122b8c60)
-Kees
--
Kees Cook
Pixel Security
Re: usercopy whitelist woe in scsi_sense_cache
On Mon, Apr 16, 2018 at 1:44 PM, Kees Cook wrote:
> On Thu, Apr 12, 2018 at 8:02 PM, Kees Cook wrote:
>> On Thu, Apr 12, 2018 at 3:47 PM, Kees Cook wrote:
>>> After fixing up some build issues in the middle of the 4.16 cycle, I
>>> get an unhelpful bisect result of commit 0a4b6e2f80aa ("Merge branch
>>> 'for-4.16/block'"). Instead of letting the test run longer, I'm going
>>> to switch to doing several shorter test boots per kernel and see if
>>> that helps. One more bisect coming...
>>
>> Okay, so I can confirm the bisect points at the _merge_ itself, not a
>> specific patch. I'm not sure how to proceed here. It looks like some
>> kind of interaction between separate trees? Jens, do you have
>> suggestions on how to track this down?
>
> Turning off HARDENED_USERCOPY and turning on KASAN, I see the same report:
>
> [ 38.274106] BUG: KASAN: slab-out-of-bounds in _copy_to_user+0x42/0x60
> [ 38.274841] Read of size 22 at addr 8800122b8c4b by task smartctl/1064
> [ 38.275630]
> [ 38.275818] CPU: 2 PID: 1064 Comm: smartctl Not tainted 4.17.0-rc1-ARCH+
> #266
> [ 38.276631] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009),
> BIOS Ubuntu-1.8.2-1ubuntu1 04/01/2014
> [ 38.277690] Call Trace:
> [ 38.277988] dump_stack+0x71/0xab
> [ 38.278397] ? _copy_to_user+0x42/0x60
> [ 38.278833] print_address_description+0x6a/0x270
> [ 38.279368] ? _copy_to_user+0x42/0x60
> [ 38.279800] kasan_report+0x243/0x360
> [ 38.280221] _copy_to_user+0x42/0x60
> [ 38.280635] sg_io+0x459/0x660
> ...
>
> Though we get slightly more details (some we already knew):
>
> [ 38.301330] Allocated by task 329:
> [ 38.301734] kmem_cache_alloc_node+0xca/0x220
> [ 38.302239] scsi_mq_init_request+0x64/0x130 [scsi_mod]
> [ 38.302821] blk_mq_alloc_rqs+0x2cf/0x370
> [ 38.303265] blk_mq_sched_alloc_tags.isra.4+0x7d/0xb0
> [ 38.303820] blk_mq_init_sched+0xf0/0x220
> [ 38.304268] elevator_switch+0x17a/0x2c0
> [ 38.304705] elv_iosched_store+0x173/0x220
> [ 38.305171] queue_attr_store+0x72/0xb0
> [ 38.305602] kernfs_fop_write+0x188/0x220
> [ 38.306049] __vfs_write+0xb6/0x330
> [ 38.306436] vfs_write+0xe9/0x240
> [ 38.306804] ksys_write+0x98/0x110
> [ 38.307181] do_syscall_64+0x6d/0x1d0
> [ 38.307590] entry_SYSCALL_64_after_hwframe+0x44/0xa9
> [ 38.308142]
> [ 38.308316] Freed by task 0:
> [ 38.308652] (stack is not available)
> [ 38.309060]
> [ 38.309243] The buggy address belongs to the object at 8800122b8c00
> [ 38.309243] which belongs to the cache scsi_sense_cache of size 96
> [ 38.310625] The buggy address is located 75 bytes inside of
> [ 38.310625] 96-byte region [8800122b8c00, 8800122b8c60)
With a hardware watchpoint, I've isolated the corruption to here:
bfq_dispatch_request+0x2be/0x1610:
__bfq_dispatch_request at block/bfq-iosched.c:3902
3900if (rq) {
3901inc_in_driver_start_rq:
3902bfqd->rq_in_driver++;
3903start_rq:
3904rq->rq_flags |= RQF_STARTED;
3905}
Through some race condition(?), rq_in_driver is also sense_buffer, and
it can get incremented. Specifically, I am doing:
diff --git a/block/blk-mq-sched.c b/block/blk-mq-sched.c
index 25c14c58385c..f50d5053d732 100644
--- a/block/blk-mq-sched.c
+++ b/block/blk-mq-sched.c
@@ -6,6 +6,8 @@
#include
#include
#include
+#include
+#include
#include
@@ -428,6 +430,18 @@ void blk_mq_sched_restart(struct blk_mq_hw_ctx *const hctx)
}
}
+static void sample_hbp_handler(struct perf_event *bp,
+ struct perf_sample_data *data,
+ struct pt_regs *regs)
+{
+printk(KERN_INFO "sense_buffer value is changed\n");
+dump_stack();
+printk(KERN_INFO "Dump stack from sample_hbp_handler\n");
+}
+
+struct perf_event * __percpu *sample_hbp;
+int ok_to_go;
+
void blk_mq_sched_insert_request(struct request *rq, bool at_head,
bool run_queue, bool async)
{
@@ -435,6 +449,16 @@ void blk_mq_sched_insert_request(struct request
*rq, bool at_head,
struct elevator_queue *e = q->elevator;
struct blk_mq_ctx *ctx = rq->mq_ctx;
struct blk_mq_hw_ctx *hctx = blk_mq_map_queue(q, ctx->cpu);
+ struct perf_event_attr attr;
+ struct scsi_request *req = scsi_req(rq);
+
+ if (ok_to_go) {
+ hw_breakpoint_init(&attr);
+ attr.bp_addr = (uintptr_t)&(req->sense);
+ attr.bp_len = HW_BREAKPOINT_LEN_8;
+ attr.bp_type = HW_BREAKPOINT_W;
+ sample_hbp = register_wide_hw_breakpoint(&attr,
sample_hbp_handler, NULL);
+
Re: usercopy whitelist woe in scsi_sense_cache
On Tue, Apr 17, 2018 at 2:19 AM, Oleksandr Natalenko wrote: > By any chance, have you tried to simplify the reproducer environment, or it > still needs my complex layout to trigger things even with KASAN? I haven't tried minimizing the reproducer yet, no. Now that I have a specific place to watch in the kernel for the corruption, though, that might help. If I get stuck again today, I'll try it. -Kees -- Kees Cook Pixel Security
Re: usercopy whitelist woe in scsi_sense_cache
On Tue, Apr 17, 2018 at 3:02 AM, James Bottomley
wrote:
> On Mon, 2018-04-16 at 20:12 -0700, Kees Cook wrote:
>> I still haven't figured this out, though... any have a moment to look
>> at this?
>
> Just to let you know you're not alone ... but I can't make any sense of
> this either. The bfdq is the elevator_data, which is initialised when
> the scheduler is attached, so it shouldn't change. Is it possible to
> set a data break point on elevator_data after it's initialised and see
> if it got changed by something?
Yeah, it seems like some pointer chain is getting overwritten outside
of a lock or rcu or ?. I don't know this code well enough to guess at
where to check, though. What I find so strange is that the structure
offsets are different between bfpd's rq_in_driver field and
scsi_request's sense field, so even THAT doesn't look to be clear-cut
either:
struct bfq_data {
struct request_queue * queue;/* 0 8 */
struct list_head dispatch; /* 816 */
struct bfq_group * root_group; /*24 8 */
struct rb_root queue_weights_tree; /*32 8 */
struct rb_root group_weights_tree; /*40 8 */
intbusy_queues; /*48 4 */
intwr_busy_queues; /*52 4 */
intqueued; /*56 4 */
intrq_in_driver; /*60 4 */
...
struct scsi_request {
unsigned char __cmd[16];/* 016 */
unsigned char *cmd; /*16 8 */
short unsigned int cmd_len; /*24 2 */
/* XXX 2 bytes hole, try to pack */
intresult; /*28 4 */
unsigned int sense_len;/*32 4 */
unsigned int resid_len;/*36 4 */
intretries; /*40 4 */
/* XXX 4 bytes hole, try to pack */
void * sense; /*48 8 */
...
This is _so_ weird. :P
-Kees
--
Kees Cook
Pixel Security
Re: usercopy whitelist woe in scsi_sense_cache
On Mon, Apr 16, 2018 at 8:12 PM, Kees Cook wrote:
> With a hardware watchpoint, I've isolated the corruption to here:
>
> bfq_dispatch_request+0x2be/0x1610:
> __bfq_dispatch_request at block/bfq-iosched.c:3902
> 3900if (rq) {
> 3901inc_in_driver_start_rq:
> 3902bfqd->rq_in_driver++;
> 3903start_rq:
> 3904rq->rq_flags |= RQF_STARTED;
> 3905}
FWIW, the stacktrace here (removing the ? lines) is:
[ 34.311980] RIP: 0010:bfq_dispatch_request+0x2be/0x1610
[ 34.452491] blk_mq_do_dispatch_sched+0x1d9/0x260
[ 34.454561] blk_mq_sched_dispatch_requests+0x3da/0x4b0
[ 34.458789] __blk_mq_run_hw_queue+0xae/0x130
[ 34.460001] __blk_mq_delay_run_hw_queue+0x192/0x280
[ 34.460823] blk_mq_run_hw_queue+0x10b/0x1b0
[ 34.463240] blk_mq_sched_insert_request+0x3bd/0x4d0
[ 34.467342] blk_execute_rq+0xcf/0x140
[ 34.468483] sg_io+0x2f7/0x730
Can anyone tell me more about the memory allocation layout of the
various variables here? It looks like struct request is a header in
front of struct scsi_request? How do struct elevator_queue, struct
blk_mq_ctx, and struct blk_mq_hw_ctx overlap these?
Regardless, I'll check for elevator data changing too...
-Kees
--
Kees Cook
Pixel Security
Re: usercopy whitelist woe in scsi_sense_cache
On Mon, Apr 16, 2018 at 8:12 PM, Kees Cook wrote:
> With a hardware watchpoint, I've isolated the corruption to here:
>
> bfq_dispatch_request+0x2be/0x1610:
> __bfq_dispatch_request at block/bfq-iosched.c:3902
> 3900if (rq) {
> 3901inc_in_driver_start_rq:
> 3902bfqd->rq_in_driver++;
> 3903start_rq:
> 3904rq->rq_flags |= RQF_STARTED;
> 3905}
This state continues to appear to be "impossible".
[ 68.845979] watchpoint triggered
[ 68.846462] sense before:8b8f9f6aae00 after:8b8f9f6aae01
[ 68.847196] elevator before:8b8f9a2c2000 after:8b8f9a2c2000
[ 68.847905] elevator_data before:8b8f9a2c0400 after:8b8f9a2c0400
...
[ 68.856925] RIP: 0010:bfq_dispatch_request+0x99/0xad0
[ 68.857553] RSP: 0018:900280c63a58 EFLAGS: 0082
[ 68.858253] RAX: 8b8f9aefbe80 RBX: 8b8f9a2c0400 RCX: 8b8f9a2c0408
[ 68.859201] RDX: 8b8f9a2c0408 RSI: 900280c63b34 RDI: 0001
[ 68.860147] RBP: R08: 000f0204 R09:
[ 68.861122] R10: 900280c63af0 R11: 0040 R12: 8b8f9aefbe00
[ 68.862089] R13: 8b8f9a221950 R14: R15: 8b8f9a2c0770
Here we can see that sense buffer has, as we've seen, been
incremented. However, the "before" values for elevator and
elevator_data still match their expected values. As such, this should
be "impossible", since:
static struct request *__bfq_dispatch_request(struct blk_mq_hw_ctx *hctx)
{
struct bfq_data *bfqd = hctx->queue->elevator->elevator_data;
...
if (rq) {
inc_in_driver_start_rq:
bfqd->rq_in_driver++;
start_rq:
rq->rq_flags |= RQF_STARTED;
}
exit:
return rq;
}
For rq_in_driver++ to touch sense, bfqd must be equal to scsi_request
- 12 bytes (rq_in_driver is 60 byte offset from struct bfq_data, and
sense is 48 bytes offset from struct scsi_request).
The above bfq_dispatch_request+0x99/0xad0 is still
__bfq_dispatch_request at block/bfq-iosched.c:3902, just with KASAN
removed. 0x99 is 153 decimal:
(gdb) disass bfq_dispatch_request
Dump of assembler code for function bfq_dispatch_request:
...
0x8134b2ad <+141>: test %rax,%rax
0x8134b2b0 <+144>: je 0x8134b2bd
0x8134b2b2 <+146>: addl $0x1,0x100(%rax)
0x8134b2b9 <+153>: addl $0x1,0x3c(%rbx)
0x8134b2bd <+157>: orl$0x2,0x18(%r12)
0x8134b2c3 <+163>: test %ebp,%ebp
0x8134b2c5 <+165>: je 0x8134b2ce
0x8134b2c7 <+167>: mov0x108(%r14),%rax
0x8134b2ce <+174>: mov%r15,%rdi
0x8134b2d1 <+177>: callq 0x81706f90 <_raw_spin_unlock_irq>
Just as a sanity-check, at +157 %r12 should be rq, rq_flags is 0x18
offset from, $0x2 is RQF_STARTED, so that maps to "rq->rq_flags |=
RQF_STARTED", the next C statement. I don't know what +146 is, though?
An increment of something 256 bytes offset? There's a lot of inline
fun and reordering happening here, so I'm ignoring that for the
moment.
So at +153 %rbx should be bfqd (i.e. elevator_data), but this is the
tripped instruction. The watchpoint dump shows RBX as 8b8f9a2c0400
... which matches elevator_data.
So, what can this be? Some sort of cache issue? By the time the
watchpoint handler captures the register information, it's already
masked the problem? I'm stumped again. :(
-Kees
--
Kees Cook
Pixel Security
Re: usercopy whitelist woe in scsi_sense_cache
On Tue, Apr 17, 2018 at 1:03 PM, Kees Cook wrote: > The above bfq_dispatch_request+0x99/0xad0 is still > __bfq_dispatch_request at block/bfq-iosched.c:3902, just with KASAN > removed. 0x99 is 153 decimal: > > (gdb) disass bfq_dispatch_request > Dump of assembler code for function bfq_dispatch_request: > ... >0x8134b2ad <+141>: test %rax,%rax >0x8134b2b0 <+144>: je 0x8134b2bd > >0x8134b2b2 <+146>: addl $0x1,0x100(%rax) >0x8134b2b9 <+153>: addl $0x1,0x3c(%rbx) >0x8134b2bd <+157>: orl$0x2,0x18(%r12) >0x8134b2c3 <+163>: test %ebp,%ebp >0x8134b2c5 <+165>: je 0x8134b2ce > >0x8134b2c7 <+167>: mov0x108(%r14),%rax >0x8134b2ce <+174>: mov%r15,%rdi >0x8134b2d1 <+177>: callq 0x81706f90 > <_raw_spin_unlock_irq> > > Just as a sanity-check, at +157 %r12 should be rq, rq_flags is 0x18 > offset from, $0x2 is RQF_STARTED, so that maps to "rq->rq_flags |= > RQF_STARTED", the next C statement. I don't know what +146 is, though? > An increment of something 256 bytes offset? There's a lot of inline > fun and reordering happening here, so I'm ignoring that for the > moment. No -- I'm reading this wrong. The RIP is the IP _after_ the trap, so +146 is the offender. [ 29.284746] watchpoint @ 95d41a0fe580 triggered [ 29.285349] sense before:95d41f45f700 after:95d41f45f701 (@95d41a 0fe580) [ 29.286176] elevator before:95d419419c00 after:95d419419c00 [ 29.286847] elevator_data before:95d419418c00 after:95d419418c00 ... [ 29.295069] RIP: 0010:bfq_dispatch_request+0x99/0xbb0 [ 29.295622] RSP: 0018:b26e01707a40 EFLAGS: 0002 [ 29.296181] RAX: ffff95d41a0fe480 RBX: 95d419418c00 RCX: 95d419418c08 RAX is 95d41a0fe480 and sense is stored at 95d41a0fe580, exactly 0x100 away. WTF is this addl? -Kees -- Kees Cook Pixel Security
Re: usercopy whitelist woe in scsi_sense_cache
On Tue, Apr 17, 2018 at 1:20 PM, Kees Cook wrote:
> On Tue, Apr 17, 2018 at 1:03 PM, Kees Cook wrote:
>> The above bfq_dispatch_request+0x99/0xad0 is still
>> __bfq_dispatch_request at block/bfq-iosched.c:3902, just with KASAN
>> removed. 0x99 is 153 decimal:
>>
>> (gdb) disass bfq_dispatch_request
>> Dump of assembler code for function bfq_dispatch_request:
>> ...
>>0x8134b2ad <+141>: test %rax,%rax
>>0x8134b2b0 <+144>: je 0x8134b2bd
>>
>>0x8134b2b2 <+146>: addl $0x1,0x100(%rax)
>>0x8134b2b9 <+153>: addl $0x1,0x3c(%rbx)
>>0x8134b2bd <+157>: orl$0x2,0x18(%r12)
>>0x8134b2c3 <+163>: test %ebp,%ebp
>>0x8134b2c5 <+165>: je 0x8134b2ce
>>
>>0x8134b2c7 <+167>: mov0x108(%r14),%rax
>>0x8134b2ce <+174>: mov%r15,%rdi
>>0x8134b2d1 <+177>: callq 0x81706f90
>> <_raw_spin_unlock_irq>
>>
>> Just as a sanity-check, at +157 %r12 should be rq, rq_flags is 0x18
>> offset from, $0x2 is RQF_STARTED, so that maps to "rq->rq_flags |=
>> RQF_STARTED", the next C statement. I don't know what +146 is, though?
>> An increment of something 256 bytes offset? There's a lot of inline
>> fun and reordering happening here, so I'm ignoring that for the
>> moment.
>
> No -- I'm reading this wrong. The RIP is the IP _after_ the trap, so
> +146 is the offender.
>
> [ 29.284746] watchpoint @ 95d41a0fe580 triggered
> [ 29.285349] sense before:95d41f45f700 after:95d41f45f701
> (@95d41a
> 0fe580)
> [ 29.286176] elevator before:95d419419c00 after:95d419419c00
> [ 29.286847] elevator_data before:95d419418c00 after:95d419418c00
> ...
> [ 29.295069] RIP: 0010:bfq_dispatch_request+0x99/0xbb0
> [ 29.295622] RSP: 0018:b26e01707a40 EFLAGS: 0002
> [ 29.296181] RAX: 95d41a0fe480 RBX: 95d419418c00 RCX:
> 95d419418c08
>
> RAX is 95d41a0fe480 and sense is stored at 95d41a0fe580,
> exactly 0x100 away.
>
> WTF is this addl?
What are the chances? :P Two ++ statements in a row separate by a
collapsed goto. FML. :)
...
bfqq->dispatched++;
goto inc_in_driver_start_rq;
...
inc_in_driver_start_rq:
bfqd->rq_in_driver++;
...
And there's the 0x100 (256):
struct bfq_queue {
...
intdispatched; /* 256 4 */
So bfqq is corrupted somewhere... I'll keep digging. I hope you're all
enjoying my live debugging transcript. ;)
-Kees
--
Kees Cook
Pixel Security
Re: usercopy whitelist woe in scsi_sense_cache
On Tue, Apr 17, 2018 at 1:28 PM, Jens Axboe wrote:
> It has to be the latter bfqq->dispatched increment, as those are
> transient (and bfqd is not).
Yeah, and I see a lot of comments around the lifetime of rq and bfqq,
so I assume something is not being locked correctly.
#define RQ_BFQQ(rq) ((rq)->elv.priv[1])
static struct request *__bfq_dispatch_request(struct blk_mq_hw_ctx *hctx)
{
struct bfq_data *bfqd = hctx->queue->elevator->elevator_data;
struct request *rq = NULL;
struct bfq_queue *bfqq = NULL;
if (!list_empty(&bfqd->dispatch)) {
rq = list_first_entry(&bfqd->dispatch, struct request,
queuelist);
list_del_init(&rq->queuelist);
bfqq = RQ_BFQQ(rq);
if (bfqq) {
/*
* Increment counters here, because this
* dispatch does not follow the standard
* dispatch flow (where counters are
* incremented)
*/
bfqq->dispatched++;
...
I see elv.priv[1] assignments made in a few places -- is it possible
there is some kind of uninitialized-but-not-NULL state that can leak
in there?
bfq_prepare_request() assigns elv.priv[1], and bfq_insert_request()
only checks that it's non-NULL (if at all) in one case. Can
bfq_insert_request() get called without bfq_prepare_request() being
called first?
-Kees
--
Kees Cook
Pixel Security
Re: usercopy whitelist woe in scsi_sense_cache
On Tue, Apr 17, 2018 at 1:46 PM, Kees Cook wrote:
> I see elv.priv[1] assignments made in a few places -- is it possible
> there is some kind of uninitialized-but-not-NULL state that can leak
> in there?
Got it. This fixes it for me:
diff --git a/block/blk-mq.c b/block/blk-mq.c
index 0dc9e341c2a7..859df3160303 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -363,7 +363,7 @@ static struct request *blk_mq_get_request(struct
request_queue *q,
rq = blk_mq_rq_ctx_init(data, tag, op);
if (!op_is_flush(op)) {
- rq->elv.icq = NULL;
+ memset(&rq->elv, 0, sizeof(rq->elv));
if (e && e->type->ops.mq.prepare_request) {
if (e->type->icq_cache && rq_ioc(bio))
blk_mq_sched_assign_ioc(rq, bio);
@@ -461,7 +461,7 @@ void blk_mq_free_request(struct request *rq)
e->type->ops.mq.finish_request(rq);
if (rq->elv.icq) {
put_io_context(rq->elv.icq->ioc);
- rq->elv.icq = NULL;
+ memset(&rq->elv, 0, sizeof(rq->elv));
}
}
--
Kees Cook
Pixel Security
[PATCH] blk-mq: Clear out elevator private data
Some elevators may not correctly check rq->rq_flags & RQF_ELVPRIV, and
may attempt to read rq->elv fields. When requests got reused, this
caused BFQ to think it already had a bfqq (rq->elv.priv[1]) allocated.
This could lead to odd behaviors like having the sense buffer address
slowly start incrementing. This eventually tripped HARDENED_USERCOPY
and KASAN.
This patch wipes all of rq->elv instead of just rq->elv.icq. While
it shouldn't technically be needed, this ends up being a robustness
improvement that should lead to at least finding bugs in elevators faster.
Reported-by: Oleksandr Natalenko
Fixes: bd166ef183c26 ("blk-mq-sched: add framework for MQ capable IO
schedulers")
Cc: sta...@vger.kernel.org
Signed-off-by: Kees Cook
---
In theory, BFQ needs to also check the RQF_ELVPRIV flag, but I'll leave that
to Paolo to figure out. Also, my Fixes line is kind of a best-guess. This
is where icq was originally wiped, so it seemed as good a commit as any.
---
block/blk-mq.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/block/blk-mq.c b/block/blk-mq.c
index 0dc9e341c2a7..859df3160303 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -363,7 +363,7 @@ static struct request *blk_mq_get_request(struct
request_queue *q,
rq = blk_mq_rq_ctx_init(data, tag, op);
if (!op_is_flush(op)) {
- rq->elv.icq = NULL;
+ memset(&rq->elv, 0, sizeof(rq->elv));
if (e && e->type->ops.mq.prepare_request) {
if (e->type->icq_cache && rq_ioc(bio))
blk_mq_sched_assign_ioc(rq, bio);
@@ -461,7 +461,7 @@ void blk_mq_free_request(struct request *rq)
e->type->ops.mq.finish_request(rq);
if (rq->elv.icq) {
put_io_context(rq->elv.icq->ioc);
- rq->elv.icq = NULL;
+ memset(&rq->elv, 0, sizeof(rq->elv));
}
}
--
2.7.4
--
Kees Cook
Pixel Security
Re: usercopy whitelist woe in scsi_sense_cache
On Tue, Apr 17, 2018 at 2:39 PM, Jens Axboe wrote:
> On 4/17/18 3:25 PM, Kees Cook wrote:
>> On Tue, Apr 17, 2018 at 1:46 PM, Kees Cook wrote:
>>> I see elv.priv[1] assignments made in a few places -- is it possible
>>> there is some kind of uninitialized-but-not-NULL state that can leak
>>> in there?
>>
>> Got it. This fixes it for me:
>>
>> diff --git a/block/blk-mq.c b/block/blk-mq.c
>> index 0dc9e341c2a7..859df3160303 100644
>> --- a/block/blk-mq.c
>> +++ b/block/blk-mq.c
>> @@ -363,7 +363,7 @@ static struct request *blk_mq_get_request(struct
>> request_queue *q,
>>
>> rq = blk_mq_rq_ctx_init(data, tag, op);
>> if (!op_is_flush(op)) {
>> - rq->elv.icq = NULL;
>> + memset(&rq->elv, 0, sizeof(rq->elv));
>> if (e && e->type->ops.mq.prepare_request) {
>> if (e->type->icq_cache && rq_ioc(bio))
>> blk_mq_sched_assign_ioc(rq, bio);
>> @@ -461,7 +461,7 @@ void blk_mq_free_request(struct request *rq)
>> e->type->ops.mq.finish_request(rq);
>> if (rq->elv.icq) {
>> put_io_context(rq->elv.icq->ioc);
>> - rq->elv.icq = NULL;
>> + memset(&rq->elv, 0, sizeof(rq->elv));
>> }
>> }
>
> This looks like a BFQ problem, this should not be necessary. Paolo,
> you're calling your own prepare request handler from the insert
> as well, and your prepare request does nothing if rq->elv.icq == NULL.
I sent the patch anyway, since it's kind of a robustness improvement,
I'd hope. If you fix BFQ also, please add:
Reported-by: Oleksandr Natalenko
Root-caused-by: Kees Cook
:) I gotta task-switch to other things!
Thanks for the pointers, and thank you Oleksandr for providing the reproducer!
-Kees
--
Kees Cook
Pixel Security
Re: [PATCH] blk-mq: Clear out elevator private data
On Tue, Apr 17, 2018 at 2:45 PM, Jens Axboe wrote: > On 4/17/18 3:42 PM, Kees Cook wrote: >> Some elevators may not correctly check rq->rq_flags & RQF_ELVPRIV, and >> may attempt to read rq->elv fields. When requests got reused, this >> caused BFQ to think it already had a bfqq (rq->elv.priv[1]) allocated. >> This could lead to odd behaviors like having the sense buffer address >> slowly start incrementing. This eventually tripped HARDENED_USERCOPY >> and KASAN. >> >> This patch wipes all of rq->elv instead of just rq->elv.icq. While >> it shouldn't technically be needed, this ends up being a robustness >> improvement that should lead to at least finding bugs in elevators faster. > > Comments from the other email still apply, we should not need to do this > full memset() for every request. From a quick look, BFQ needs to straighten > out its usage of prepare request and interactions with insert_request. Sure, understood. I would point out, FWIW, that memset() gets unrolled by the compiler and this is just two more XORs in the same cacheline (the two words following icq). (And there is SO much more being cleared during alloc, it didn't seem like hardly any extra cost vs the robustness it provided.) -Kees -- Kees Cook Pixel Security
Re: usercopy whitelist woe in scsi_sense_cache
On Tue, Apr 17, 2018 at 3:57 PM, Jens Axboe wrote:
> On 4/17/18 3:48 PM, Jens Axboe wrote:
>> On 4/17/18 3:47 PM, Kees Cook wrote:
>>> On Tue, Apr 17, 2018 at 2:39 PM, Jens Axboe wrote:
>>>> On 4/17/18 3:25 PM, Kees Cook wrote:
>>>>> On Tue, Apr 17, 2018 at 1:46 PM, Kees Cook wrote:
>>>>>> I see elv.priv[1] assignments made in a few places -- is it possible
>>>>>> there is some kind of uninitialized-but-not-NULL state that can leak
>>>>>> in there?
>>>>>
>>>>> Got it. This fixes it for me:
>>>>>
>>>>> diff --git a/block/blk-mq.c b/block/blk-mq.c
>>>>> index 0dc9e341c2a7..859df3160303 100644
>>>>> --- a/block/blk-mq.c
>>>>> +++ b/block/blk-mq.c
>>>>> @@ -363,7 +363,7 @@ static struct request *blk_mq_get_request(struct
>>>>> request_queue *q,
>>>>>
>>>>> rq = blk_mq_rq_ctx_init(data, tag, op);
>>>>> if (!op_is_flush(op)) {
>>>>> - rq->elv.icq = NULL;
>>>>> + memset(&rq->elv, 0, sizeof(rq->elv));
>>>>> if (e && e->type->ops.mq.prepare_request) {
>>>>> if (e->type->icq_cache && rq_ioc(bio))
>>>>> blk_mq_sched_assign_ioc(rq, bio);
>>>>> @@ -461,7 +461,7 @@ void blk_mq_free_request(struct request *rq)
>>>>> e->type->ops.mq.finish_request(rq);
>>>>> if (rq->elv.icq) {
>>>>> put_io_context(rq->elv.icq->ioc);
>>>>> - rq->elv.icq = NULL;
>>>>> + memset(&rq->elv, 0, sizeof(rq->elv));
>>>>> }
>>>>> }
>>>>
>>>> This looks like a BFQ problem, this should not be necessary. Paolo,
>>>> you're calling your own prepare request handler from the insert
>>>> as well, and your prepare request does nothing if rq->elv.icq == NULL.
>>>
>>> I sent the patch anyway, since it's kind of a robustness improvement,
>>> I'd hope. If you fix BFQ also, please add:
>>
>> It's also a memset() in the hot path, would prefer to avoid that...
>> The issue here is really the convoluted bfq usage of insert/prepare,
>> I'm sure Paolo can take it from here.
>
> Does this fix it?
>
> diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
> index f0ecd98509d8..d883469a1582 100644
> --- a/block/bfq-iosched.c
> +++ b/block/bfq-iosched.c
> @@ -4934,8 +4934,11 @@ static void bfq_prepare_request(struct request *rq,
> struct bio *bio)
> bool new_queue = false;
> bool bfqq_already_existing = false, split = false;
>
> - if (!rq->elv.icq)
> + if (!rq->elv.icq) {
> + rq->elv.priv[0] = rq->elv.priv[1] = NULL;
> return;
> + }
> +
> bic = icq_to_bic(rq->elv.icq);
>
> spin_lock_irq(&bfqd->lock);
It does! Excellent. :)
-Kees
--
Kees Cook
Pixel Security
Re: usercopy whitelist woe in scsi_sense_cache
On Thu, Apr 19, 2018 at 2:32 AM, Paolo Valente wrote: > I'm missing something here. When the request gets completed in the > first place, the hook bfq_finish_requeue_request gets called, and that > hook clears both ->elv.priv elements (as the request has a non-null > elv.icq). So, when bfq gets the same request again, those elements > must be NULL. What am I getting wrong? > > I have some more concern on this point, but I'll stick to this for the > moment, to not create more confusion. I don't know the "how", I only found the "what". :) If you want, grab the reproducer VM linked to earlier in this thread; it'll hit the problem within about 30 seconds of running the reproducer. -Kees -- Kees Cook Pixel Security
[PATCH] scsi: dpt_i2o: Remove VLA usage
On the quest to remove all VLAs from the kernel[1] this moves the sg_list
variable off the stack, as already done for other allocated buffers in
adpt_i2o_passthru(). Additionally consolidates the error path for kfree().
[1]
https://lkml.kernel.org/r/CA+55aFzCG-zNmZwX4A2FQpadafLfEzK6CC=qpxydaacu1rq...@mail.gmail.com
Signed-off-by: Kees Cook
---
drivers/scsi/dpt_i2o.c | 21 ++---
1 file changed, 14 insertions(+), 7 deletions(-)
diff --git a/drivers/scsi/dpt_i2o.c b/drivers/scsi/dpt_i2o.c
index 5ceea8da7bb6..37de8fb186d7 100644
--- a/drivers/scsi/dpt_i2o.c
+++ b/drivers/scsi/dpt_i2o.c
@@ -1706,7 +1706,7 @@ static int adpt_i2o_passthru(adpt_hba* pHba, u32 __user
*arg)
u32 reply_size = 0;
u32 __user *user_msg = arg;
u32 __user * user_reply = NULL;
- void *sg_list[pHba->sg_tablesize];
+ void **sg_list = NULL;
u32 sg_offset = 0;
u32 sg_count = 0;
int sg_index = 0;
@@ -1748,19 +1748,23 @@ static int adpt_i2o_passthru(adpt_hba* pHba, u32 __user
*arg)
msg[2] = 0x4000; // IOCTL context
msg[3] = adpt_ioctl_to_context(pHba, reply);
if (msg[3] == (u32)-1) {
- kfree(reply);
- return -EBUSY;
+ rcode = -EBUSY;
+ goto free;
}
- memset(sg_list,0, sizeof(sg_list[0])*pHba->sg_tablesize);
+ sg_list = kcalloc(pHba->sg_tablesize, sizeof(*sg_list), GFP_KERNEL);
+ if (!sg_list) {
+ rcode = -ENOMEM;
+ goto free;
+ }
if(sg_offset) {
// TODO add 64 bit API
struct sg_simple_element *sg = (struct sg_simple_element*)
(msg+sg_offset);
sg_count = (size - sg_offset*4) / sizeof(struct
sg_simple_element);
if (sg_count > pHba->sg_tablesize){
printk(KERN_DEBUG"%s:IOCTL SG List too large (%u)\n",
pHba->name,sg_count);
- kfree (reply);
- return -EINVAL;
+ rcode = -EINVAL;
+ goto free;
}
for(i = 0; i < sg_count; i++) {
@@ -1879,7 +1883,6 @@ static int adpt_i2o_passthru(adpt_hba* pHba, u32 __user
*arg)
if (rcode != -ETIME && rcode != -EINTR) {
struct sg_simple_element *sg =
(struct sg_simple_element*) (msg +sg_offset);
- kfree (reply);
while(sg_index) {
if(sg_list[--sg_index]) {
dma_free_coherent(&pHba->pDev->dev,
@@ -1889,6 +1892,10 @@ static int adpt_i2o_passthru(adpt_hba* pHba, u32 __user
*arg)
}
}
}
+
+free:
+ kfree(sg_list);
+ kfree(reply);
return rcode;
}
--
2.17.0
--
Kees Cook
Pixel Security
[PATCH] scsi: libosd: Remove VLA usage
On the quest to remove all VLAs from the kernel[1] this rearranges the
code to avoid a VLA warning under -Wvla (gcc doesn't recognize "const"
variables as not triggering VLA creation). Additionally cleans up variable
naming to avoid 80 character column limit.
[1]
https://lkml.kernel.org/r/CA+55aFzCG-zNmZwX4A2FQpadafLfEzK6CC=qpxydaacu1rq...@mail.gmail.com
Signed-off-by: Kees Cook
---
drivers/scsi/osd/osd_initiator.c | 16
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/drivers/scsi/osd/osd_initiator.c b/drivers/scsi/osd/osd_initiator.c
index e18877177f1b..917a86a2ae8c 100644
--- a/drivers/scsi/osd/osd_initiator.c
+++ b/drivers/scsi/osd/osd_initiator.c
@@ -1842,14 +1842,14 @@ int osd_req_decode_sense_full(struct osd_request *or,
case osd_sense_response_integrity_check:
{
struct osd_sense_response_integrity_check_descriptor
- *osricd = cur_descriptor;
- const unsigned len =
- sizeof(osricd->integrity_check_value);
- char key_dump[len*4 + 2]; /* 2nibbles+space+ASCII */
-
- hex_dump_to_buffer(osricd->integrity_check_value, len,
- 32, 1, key_dump, sizeof(key_dump), true);
- OSD_SENSE_PRINT2("response_integrity [%s]\n", key_dump);
+ *d = cur_descriptor;
+ /* 2nibbles+space+ASCII */
+ char dump[sizeof(d->integrity_check_value) * 4 + 2];
+
+ hex_dump_to_buffer(d->integrity_check_value,
+ sizeof(d->integrity_check_value),
+ 32, 1, dump, sizeof(dump), true);
+ OSD_SENSE_PRINT2("response_integrity [%s]\n", dump);
}
case osd_sense_attribute_identification:
{
--
2.17.0
--
Kees Cook
Pixel Security
[PATCH] scsi: ufs: ufshcd: Remove VLA usage
On the quest to remove all VLAs from the kernel[1] this moves buffers
off the stack. In the second instance, this collapses two separately
allocated buffers into a single buffer, since they are used consecutively,
which saves 256 bytes (QUERY_DESC_MAX_SIZE + 1) of stack space.
[1]
https://lkml.kernel.org/r/CA+55aFzCG-zNmZwX4A2FQpadafLfEzK6CC=qpxydaacu1rq...@mail.gmail.com
Signed-off-by: Kees Cook
---
drivers/scsi/ufs/ufshcd.c | 34 ++
1 file changed, 26 insertions(+), 8 deletions(-)
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 00e79057f870..a271534362f6 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -5958,14 +5958,18 @@ static void ufshcd_init_icc_levels(struct ufs_hba *hba)
{
int ret;
int buff_len = hba->desc_size.pwr_desc;
- u8 desc_buf[hba->desc_size.pwr_desc];
+ u8 *desc_buf;
+
+ desc_buf = kmalloc(buff_len, GFP_KERNEL);
+ if (!desc_buf)
+ return;
ret = ufshcd_read_power_desc(hba, desc_buf, buff_len);
if (ret) {
dev_err(hba->dev,
"%s: Failed reading power descriptor.len = %d ret = %d",
__func__, buff_len, ret);
- return;
+ goto out;
}
hba->init_prefetch_data.icc_level =
@@ -5983,6 +5987,8 @@ static void ufshcd_init_icc_levels(struct ufs_hba *hba)
"%s: Failed configuring bActiveICCLevel = %d ret = %d",
__func__, hba->init_prefetch_data.icc_level , ret);
+out:
+ kfree(desc_buf);
}
/**
@@ -6052,9 +6058,17 @@ static int ufs_get_device_desc(struct ufs_hba *hba,
struct ufs_dev_desc *dev_desc)
{
int err;
+ size_t buff_len;
u8 model_index;
- u8 str_desc_buf[QUERY_DESC_MAX_SIZE + 1] = {0};
- u8 desc_buf[hba->desc_size.dev_desc];
+ u8 *desc_buf;
+
+ buff_len = max_t(size_t, hba->desc_size.dev_desc,
+QUERY_DESC_MAX_SIZE + 1);
+ desc_buf = kmalloc(buff_len, GFP_KERNEL);
+ if (!desc_buf) {
+ err = -ENOMEM;
+ goto out;
+ }
err = ufshcd_read_device_desc(hba, desc_buf, hba->desc_size.dev_desc);
if (err) {
@@ -6072,7 +6086,10 @@ static int ufs_get_device_desc(struct ufs_hba *hba,
model_index = desc_buf[DEVICE_DESC_PARAM_PRDCT_NAME];
- err = ufshcd_read_string_desc(hba, model_index, str_desc_buf,
+ /* Zero-pad entire buffer for string termination. */
+ memset(desc_buf, 0, buff_len);
+
+ err = ufshcd_read_string_desc(hba, model_index, desc_buf,
QUERY_DESC_MAX_SIZE, true/*ASCII*/);
if (err) {
dev_err(hba->dev, "%s: Failed reading Product Name. err = %d\n",
@@ -6080,15 +6097,16 @@ static int ufs_get_device_desc(struct ufs_hba *hba,
goto out;
}
- str_desc_buf[QUERY_DESC_MAX_SIZE] = '\0';
- strlcpy(dev_desc->model, (str_desc_buf + QUERY_DESC_HDR_SIZE),
- min_t(u8, str_desc_buf[QUERY_DESC_LENGTH_OFFSET],
+ desc_buf[QUERY_DESC_MAX_SIZE] = '\0';
+ strlcpy(dev_desc->model, (desc_buf + QUERY_DESC_HDR_SIZE),
+ min_t(u8, desc_buf[QUERY_DESC_LENGTH_OFFSET],
MAX_MODEL_LEN));
/* Null terminate the model string */
dev_desc->model[MAX_MODEL_LEN] = '\0';
out:
+ kfree(desc_buf);
return err;
}
--
2.17.0
--
Kees Cook
Pixel Security
Re: [PATCH v6 05/13] firmware_loader: enhance Kconfig documentation over FW_LOADER
is feature enabled is to support a > + driver which explicitly relies on this fallback mechanism. Only two > + drivers need this today: > + > + o CONFIG_LEDS_LP55XX_COMMON > + o CONFIG_DELL_RBU > + > + Outside of supporting the above drivers, another reason for needing > + this may be that your firmware resides outside of the paths the > kernel > + looks for and cannot possibily be specified using the firmware_class > + path module parameter or kernel firmware_class path boot parameter > + if firmware_class is built-in. > + > + A modern use case may be to temporarily mount a custom partition > + during provisioning which is only accessible to userspace, and then > + to use it to look for and fetch the required firmware. Such type of > + driver functionality may not even ever be desirable upstream by > + vendors, and as such is only required to be supported as an > interface > + for provisioning. Since udev's firmware loading facility has been > + removed you can use firmwared or a fork of it to customize how you > + want to load firmware based on uevents issued. > + > + Enabling this option will increase your kernel image size by about > + 13436 bytes. > + > + If you are unsure about this, say N here, unless you are Linux > + distribution and need to support the above two drivers, or you are > + certain you need to support some really custom firmware loading > + facility in userspace. > > config FW_LOADER_USER_HELPER_FALLBACK > - bool "Fallback user-helper invocation for firmware loading" > - depends on FW_LOADER > - select FW_LOADER_USER_HELPER > + bool "Force the firmware sysfs fallback mechanism when possible" > + depends on FW_LOADER_USER_HELPER > help > - This option enables / disables the invocation of user-helper > - (e.g. udev) for loading firmware files as a fallback after the > - direct file loading in kernel fails. The user-mode helper is > - no longer required unless you have a special firmware file that > - resides in a non-standard path. Moreover, the udev support has > - been deprecated upstream. > + Enabling this option forces a sysfs userspace fallback mechanism > + to be used for all firmware requests which explicitly do not > disable a > + a fallback mechanism. Firmware calls which do prohibit a fallback > + mechanism is request_firmware_direct(). This option is kept for > + backward compatibility purposes given this precise mechanism can > also > + be enabled by setting the proc sysctl value to true: > + > + /proc/sys/kernel/firmware_config/force_sysfs_fallback > > If you are unsure about this, say N here. > > +endif # FW_LOADER > +endmenu > + > config WANT_DEV_COREDUMP > bool > help > -- > 2.17.0 > -Kees -- Kees Cook Pixel Security
Re: [PATCH v6 00/13] firmware_loader changes for v4.18
On Tue, May 8, 2018 at 11:12 AM, Luis R. Rodriguez wrote: > Greg, > > Here is what I have queued up for the firmware_loader for v4.18. It > includes a slew of cleanup work, and the new firmware_request_nowarn() > which is quiet but enables the sysfs fallback mechanism. I've gone ahead > and also queued up a few minor fixes for the firmware loader documentation > which have come up recently. These changes are available on my git tree > both based on linux-next [0] and Linus' latest tree [1]. Folks working > on new developments for the firmware loader can use my linux-next > branch 20180508-firmware_loader_for-v4.18-try2 for now. > > 0-day sends its blessings. > > The patches from Mimi's series still require a bit more discussion and > review. The discussion over the EFI firmware fallback mechanism is still > ongoing. > > As for the rename that you wanted, perhaps we can do this late in the > merge window considering we're at rc4 now. I can prep something up for > that later. > > Question, and specially rants are warmly welcomed. I sent some typo catches, but with those fixed, please consider the whole series: Reviewed-by: Kees Cook Thanks! -Kees -- Kees Cook Pixel Security
Re: [PATCH v6 05/13] firmware_loader: enhance Kconfig documentation over FW_LOADER
On Wed, May 9, 2018 at 1:55 PM, Luis R. Rodriguez wrote: > On Tue, May 08, 2018 at 03:42:33PM -0700, Kees Cook wrote: >> On Tue, May 8, 2018 at 11:12 AM, Luis R. Rodriguez wrote: >> > + This used to be the default firmware loading facility, and udev >> > used >> > + to listen for uvents to load firmware for the kernel. The >> > firmware >> > + loading facility functionality in udev has been removed, as such >> > it >> > + can no longer be relied upon as a fallback mechanism. Linux no >> > longer >> > + relies on or uses a fallback mechanism in userspace. If you need >> > to >> > + rely on one refer to the permissively licensed firmwared: >> >> Typo: firmware > > Thanks fixed all typos except this one, this one is meant to be firmwared as > that is the name of the project, the url is below. > >> >> > + >> > + https://github.com/teg/firmwared Oh! Yes, hah. :) Thanks! -Kees -- Kees Cook Pixel Security
Re: [PATCH] scsi: dpt_i2o: Remove VLA usage
On Wed, May 2, 2018 at 3:21 PM, Kees Cook wrote:
> On the quest to remove all VLAs from the kernel[1] this moves the sg_list
> variable off the stack, as already done for other allocated buffers in
> adpt_i2o_passthru(). Additionally consolidates the error path for kfree().
>
> [1]
> https://lkml.kernel.org/r/CA+55aFzCG-zNmZwX4A2FQpadafLfEzK6CC=qpxydaacu1rq...@mail.gmail.com
>
> Signed-off-by: Kees Cook
Friendly ping! How does this look for v4.18?
Thanks!
-Kees
> ---
> drivers/scsi/dpt_i2o.c | 21 ++---
> 1 file changed, 14 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/scsi/dpt_i2o.c b/drivers/scsi/dpt_i2o.c
> index 5ceea8da7bb6..37de8fb186d7 100644
> --- a/drivers/scsi/dpt_i2o.c
> +++ b/drivers/scsi/dpt_i2o.c
> @@ -1706,7 +1706,7 @@ static int adpt_i2o_passthru(adpt_hba* pHba, u32 __user
> *arg)
> u32 reply_size = 0;
> u32 __user *user_msg = arg;
> u32 __user * user_reply = NULL;
> - void *sg_list[pHba->sg_tablesize];
> + void **sg_list = NULL;
> u32 sg_offset = 0;
> u32 sg_count = 0;
> int sg_index = 0;
> @@ -1748,19 +1748,23 @@ static int adpt_i2o_passthru(adpt_hba* pHba, u32
> __user *arg)
> msg[2] = 0x4000; // IOCTL context
> msg[3] = adpt_ioctl_to_context(pHba, reply);
> if (msg[3] == (u32)-1) {
> - kfree(reply);
> - return -EBUSY;
> + rcode = -EBUSY;
> + goto free;
> }
>
> - memset(sg_list,0, sizeof(sg_list[0])*pHba->sg_tablesize);
> + sg_list = kcalloc(pHba->sg_tablesize, sizeof(*sg_list), GFP_KERNEL);
> + if (!sg_list) {
> + rcode = -ENOMEM;
> + goto free;
> + }
> if(sg_offset) {
> // TODO add 64 bit API
> struct sg_simple_element *sg = (struct sg_simple_element*)
> (msg+sg_offset);
> sg_count = (size - sg_offset*4) / sizeof(struct
> sg_simple_element);
> if (sg_count > pHba->sg_tablesize){
> printk(KERN_DEBUG"%s:IOCTL SG List too large (%u)\n",
> pHba->name,sg_count);
> - kfree (reply);
> - return -EINVAL;
> + rcode = -EINVAL;
> + goto free;
> }
>
> for(i = 0; i < sg_count; i++) {
> @@ -1879,7 +1883,6 @@ static int adpt_i2o_passthru(adpt_hba* pHba, u32 __user
> *arg)
> if (rcode != -ETIME && rcode != -EINTR) {
> struct sg_simple_element *sg =
> (struct sg_simple_element*) (msg +sg_offset);
> - kfree (reply);
> while(sg_index) {
> if(sg_list[--sg_index]) {
> dma_free_coherent(&pHba->pDev->dev,
> @@ -1889,6 +1892,10 @@ static int adpt_i2o_passthru(adpt_hba* pHba, u32
> __user *arg)
> }
> }
> }
> +
> +free:
> + kfree(sg_list);
> + kfree(reply);
> return rcode;
> }
>
> --
> 2.17.0
>
>
> --
> Kees Cook
> Pixel Security
--
Kees Cook
Pixel Security
[PATCH 4/6] block: Consolidate scsi sense buffer usage
There is a lot of needless struct request_sense usage in the CDROM
code. These can all be struct scsi_sense_hdr instead, to avoid any
confusion over their respective structure sizes. This patch is a lot
of noise changing "sense" to "sshdr", but the final code is more
readable to distinguish between "sense" meaning "struct request_sense"
and "sshdr" meaning "struct scsi_sense_hdr".
Signed-off-by: Kees Cook
---
drivers/block/pktcdvd.c| 36 ++--
drivers/cdrom/cdrom.c | 22 +++---
drivers/ide/ide-cd.c | 11 ++-
drivers/ide/ide-cd.h | 5 +++--
drivers/ide/ide-cd_ioctl.c | 30 +++---
drivers/scsi/sr_ioctl.c| 22 +-
include/linux/cdrom.h | 3 ++-
7 files changed, 64 insertions(+), 65 deletions(-)
diff --git a/drivers/block/pktcdvd.c b/drivers/block/pktcdvd.c
index c61d20c9f3f8..f91c9f85e29d 100644
--- a/drivers/block/pktcdvd.c
+++ b/drivers/block/pktcdvd.c
@@ -748,13 +748,13 @@ static const char *sense_key_string(__u8 index)
static void pkt_dump_sense(struct pktcdvd_device *pd,
struct packet_command *cgc)
{
- struct request_sense *sense = cgc->sense;
+ struct scsi_sense_hdr *sshdr = cgc->sshdr;
- if (sense)
+ if (sshdr)
pkt_err(pd, "%*ph - sense %02x.%02x.%02x (%s)\n",
CDROM_PACKET_SIZE, cgc->cmd,
- sense->sense_key, sense->asc, sense->ascq,
- sense_key_string(sense->sense_key));
+ sshdr->sense_key, sshdr->asc, sshdr->ascq,
+ sense_key_string(sshdr->sense_key));
else
pkt_err(pd, "%*ph - no sense\n", CDROM_PACKET_SIZE, cgc->cmd);
}
@@ -787,11 +787,11 @@ static noinline_for_stack int pkt_set_speed(struct
pktcdvd_device *pd,
unsigned write_speed, unsigned read_speed)
{
struct packet_command cgc;
- struct request_sense sense;
+ struct scsi_sense_hdr sshdr;
int ret;
init_cdrom_command(&cgc, NULL, 0, CGC_DATA_NONE);
- cgc.sense = &sense;
+ cgc.sshdr = &sshdr;
cgc.cmd[0] = GPCMD_SET_SPEED;
cgc.cmd[2] = (read_speed >> 8) & 0xff;
cgc.cmd[3] = read_speed & 0xff;
@@ -1645,7 +1645,7 @@ static noinline_for_stack int pkt_get_last_written(struct
pktcdvd_device *pd,
static noinline_for_stack int pkt_set_write_settings(struct pktcdvd_device *pd)
{
struct packet_command cgc;
- struct request_sense sense;
+ struct scsi_sense_hdr sshdr;
write_param_page *wp;
char buffer[128];
int ret, size;
@@ -1656,7 +1656,7 @@ static noinline_for_stack int
pkt_set_write_settings(struct pktcdvd_device *pd)
memset(buffer, 0, sizeof(buffer));
init_cdrom_command(&cgc, buffer, sizeof(*wp), CGC_DATA_READ);
- cgc.sense = &sense;
+ cgc.sshdr = &sshdr;
if ((ret = pkt_mode_sense(pd, &cgc, GPMODE_WRITE_PARMS_PAGE, 0))) {
pkt_dump_sense(pd, &cgc);
return ret;
@@ -1671,7 +1671,7 @@ static noinline_for_stack int
pkt_set_write_settings(struct pktcdvd_device *pd)
* now get it all
*/
init_cdrom_command(&cgc, buffer, size, CGC_DATA_READ);
- cgc.sense = &sense;
+ cgc.sshdr = &sshdr;
if ((ret = pkt_mode_sense(pd, &cgc, GPMODE_WRITE_PARMS_PAGE, 0))) {
pkt_dump_sense(pd, &cgc);
return ret;
@@ -1905,12 +1905,12 @@ static noinline_for_stack int pkt_write_caching(struct
pktcdvd_device *pd,
int set)
{
struct packet_command cgc;
- struct request_sense sense;
+ struct scsi_sense_hdr sshdr;
unsigned char buf[64];
int ret;
init_cdrom_command(&cgc, buf, sizeof(buf), CGC_DATA_READ);
- cgc.sense = &sense;
+ cgc.sshdr = &sshdr;
cgc.buflen = pd->mode_offset + 12;
/*
@@ -1950,14 +1950,14 @@ static noinline_for_stack int pkt_get_max_speed(struct
pktcdvd_device *pd,
unsigned *write_speed)
{
struct packet_command cgc;
- struct request_sense sense;
+ struct scsi_sense_hdr sshdr;
unsigned char buf[256+18];
unsigned char *cap_buf;
int ret, offset;
cap_buf = &buf[sizeof(struct mode_page_header) + pd->mode_offset];
init_cdrom_command(&cgc, buf, sizeof(buf), CGC_DATA_UNKNOWN);
- cgc.sense = &sense;
+ cgc.sshdr = &sshdr;
ret = pkt_mode_sense(pd, &cgc, GPMODE_CAPABILITIES_PAGE, 0);
if (ret) {
@@ -2011,13 +2011,13 @@ static noinline_for_stack int pkt_media_speed(struct
pktc
[PATCH 5/6] libata-scsi: Move sense buffers onto stack
Instead of dynamically allocating the sense buffers, put them on the
stack so that future compile-time sizeof() checks will be able to see
their buffer length.
Signed-off-by: Kees Cook
---
drivers/ata/libata-scsi.c | 18 ++
1 file changed, 6 insertions(+), 12 deletions(-)
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 89a9d4a2efc8..3a43d3a1ce2d 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -597,8 +597,9 @@ static int ata_get_identity(struct ata_port *ap, struct
scsi_device *sdev,
int ata_cmd_ioctl(struct scsi_device *scsidev, void __user *arg)
{
int rc = 0;
+ u8 sensebuf[SCSI_SENSE_BUFFERSIZE];
u8 scsi_cmd[MAX_COMMAND_SIZE];
- u8 args[4], *argbuf = NULL, *sensebuf = NULL;
+ u8 args[4], *argbuf = NULL;
int argsize = 0;
enum dma_data_direction data_dir;
struct scsi_sense_hdr sshdr;
@@ -610,10 +611,7 @@ int ata_cmd_ioctl(struct scsi_device *scsidev, void __user
*arg)
if (copy_from_user(args, arg, sizeof(args)))
return -EFAULT;
- sensebuf = kzalloc(SCSI_SENSE_BUFFERSIZE, GFP_NOIO);
- if (!sensebuf)
- return -ENOMEM;
-
+ memset(sensebuf, 0, sizeof(sensebuf));
memset(scsi_cmd, 0, sizeof(scsi_cmd));
if (args[3]) {
@@ -685,7 +683,6 @@ int ata_cmd_ioctl(struct scsi_device *scsidev, void __user
*arg)
&& copy_to_user(arg + sizeof(args), argbuf, argsize))
rc = -EFAULT;
error:
- kfree(sensebuf);
kfree(argbuf);
return rc;
}
@@ -704,8 +701,9 @@ int ata_cmd_ioctl(struct scsi_device *scsidev, void __user
*arg)
int ata_task_ioctl(struct scsi_device *scsidev, void __user *arg)
{
int rc = 0;
+ u8 sensebuf[SCSI_SENSE_BUFFERSIZE];
u8 scsi_cmd[MAX_COMMAND_SIZE];
- u8 args[7], *sensebuf = NULL;
+ u8 args[7];
struct scsi_sense_hdr sshdr;
int cmd_result;
@@ -715,10 +713,7 @@ int ata_task_ioctl(struct scsi_device *scsidev, void
__user *arg)
if (copy_from_user(args, arg, sizeof(args)))
return -EFAULT;
- sensebuf = kzalloc(SCSI_SENSE_BUFFERSIZE, GFP_NOIO);
- if (!sensebuf)
- return -ENOMEM;
-
+ memset(sensebuf, 0, sizeof(sensebuf));
memset(scsi_cmd, 0, sizeof(scsi_cmd));
scsi_cmd[0] = ATA_16;
scsi_cmd[1] = (3 << 1); /* Non-data */
@@ -769,7 +764,6 @@ int ata_task_ioctl(struct scsi_device *scsidev, void __user
*arg)
}
error:
- kfree(sensebuf);
return rc;
}
--
2.17.0
[PATCH 6/6] scsi: Check sense buffer size at build time
To avoid introducing problems like those fixed in commit f7068114d45e
("sr: pass down correctly sized SCSI sense buffer"), this creates a macro
wrapper for scsi_execute() that verifies the size of the sense buffer
similar to what was done for command string sizes in commit 3756f6401c30
("exec: avoid gcc-8 warning for get_task_comm").
Another solution could be to add another argument to scsi_execute(),
but this function already takes a lot of arguments and Jens was not fond
of that approach. As there was only a pair of dynamically allocated sense
buffers, this also moves those 96 bytes onto the stack to avoid triggering
the sizeof() check.
Signed-off-by: Kees Cook
---
drivers/scsi/scsi_lib.c| 6 +++---
include/scsi/scsi_device.h | 12 +++-
2 files changed, 14 insertions(+), 4 deletions(-)
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index e9b4f279d29c..718c2bec4516 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -238,7 +238,7 @@ void scsi_queue_insert(struct scsi_cmnd *cmd, int reason)
/**
- * scsi_execute - insert request and wait for the result
+ * __scsi_execute - insert request and wait for the result
* @sdev: scsi device
* @cmd: scsi command
* @data_direction: data direction
@@ -255,7 +255,7 @@ void scsi_queue_insert(struct scsi_cmnd *cmd, int reason)
* Returns the scsi_cmnd result field if a command was executed, or a negative
* Linux error code if we didn't get that far.
*/
-int scsi_execute(struct scsi_device *sdev, const unsigned char *cmd,
+int __scsi_execute(struct scsi_device *sdev, const unsigned char *cmd,
int data_direction, void *buffer, unsigned bufflen,
unsigned char *sense, struct scsi_sense_hdr *sshdr,
int timeout, int retries, u64 flags, req_flags_t rq_flags,
@@ -309,7 +309,7 @@ int scsi_execute(struct scsi_device *sdev, const unsigned
char *cmd,
return ret;
}
-EXPORT_SYMBOL(scsi_execute);
+EXPORT_SYMBOL(__scsi_execute);
/*
* Function:scsi_init_cmd_errh()
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index 7ae177c8e399..1bb87b6c0ad2 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -426,11 +426,21 @@ extern const char *scsi_device_state_name(enum
scsi_device_state);
extern int scsi_is_sdev_device(const struct device *);
extern int scsi_is_target_device(const struct device *);
extern void scsi_sanitize_inquiry_string(unsigned char *s, int len);
-extern int scsi_execute(struct scsi_device *sdev, const unsigned char *cmd,
+extern int __scsi_execute(struct scsi_device *sdev, const unsigned char *cmd,
int data_direction, void *buffer, unsigned bufflen,
unsigned char *sense, struct scsi_sense_hdr *sshdr,
int timeout, int retries, u64 flags,
req_flags_t rq_flags, int *resid);
+/* Make sure any sense buffer is the correct size. */
+#define scsi_execute(sdev, cmd, data_direction, buffer, bufflen, sense,
\
+sshdr, timeout, retries, flags, rq_flags, resid) \
+({ \
+ BUILD_BUG_ON((sense) != NULL && \
+sizeof(sense) != SCSI_SENSE_BUFFERSIZE); \
+ __scsi_execute(sdev, cmd, data_direction, buffer, bufflen, \
+ sense, sshdr, timeout, retries, flags, rq_flags, \
+ resid); \
+})
static inline int scsi_execute_req(struct scsi_device *sdev,
const unsigned char *cmd, int data_direction, void *buffer,
unsigned bufflen, struct scsi_sense_hdr *sshdr, int timeout,
--
2.17.0
[PATCH 2/6] scsi: cxlflash: Drop unused sense buffers
This removes the unused sense buffer in read_cap16() and write_same16().
Signed-off-by: Kees Cook
---
drivers/scsi/cxlflash/superpipe.c | 8 ++--
drivers/scsi/cxlflash/vlun.c | 7 ++-
2 files changed, 4 insertions(+), 11 deletions(-)
diff --git a/drivers/scsi/cxlflash/superpipe.c
b/drivers/scsi/cxlflash/superpipe.c
index 2fe79df5c73c..59b9f2023748 100644
--- a/drivers/scsi/cxlflash/superpipe.c
+++ b/drivers/scsi/cxlflash/superpipe.c
@@ -324,7 +324,6 @@ static int read_cap16(struct scsi_device *sdev, struct
llun_info *lli)
struct scsi_sense_hdr sshdr;
u8 *cmd_buf = NULL;
u8 *scsi_cmd = NULL;
- u8 *sense_buf = NULL;
int rc = 0;
int result = 0;
int retry_cnt = 0;
@@ -333,8 +332,7 @@ static int read_cap16(struct scsi_device *sdev, struct
llun_info *lli)
retry:
cmd_buf = kzalloc(CMD_BUFSIZE, GFP_KERNEL);
scsi_cmd = kzalloc(MAX_COMMAND_SIZE, GFP_KERNEL);
- sense_buf = kzalloc(SCSI_SENSE_BUFFERSIZE, GFP_KERNEL);
- if (unlikely(!cmd_buf || !scsi_cmd || !sense_buf)) {
+ if (unlikely(!cmd_buf || !scsi_cmd)) {
rc = -ENOMEM;
goto out;
}
@@ -349,7 +347,7 @@ static int read_cap16(struct scsi_device *sdev, struct
llun_info *lli)
/* Drop the ioctl read semahpore across lengthy call */
up_read(&cfg->ioctl_rwsem);
result = scsi_execute(sdev, scsi_cmd, DMA_FROM_DEVICE, cmd_buf,
- CMD_BUFSIZE, sense_buf, &sshdr, to, CMD_RETRIES,
+ CMD_BUFSIZE, NULL, &sshdr, to, CMD_RETRIES,
0, 0, NULL);
down_read(&cfg->ioctl_rwsem);
rc = check_state(cfg);
@@ -380,7 +378,6 @@ static int read_cap16(struct scsi_device *sdev, struct
llun_info *lli)
if (retry_cnt++ < 1) {
kfree(cmd_buf);
kfree(scsi_cmd);
- kfree(sense_buf);
goto retry;
}
}
@@ -411,7 +408,6 @@ static int read_cap16(struct scsi_device *sdev, struct
llun_info *lli)
out:
kfree(cmd_buf);
kfree(scsi_cmd);
- kfree(sense_buf);
dev_dbg(dev, "%s: maxlba=%lld blklen=%d rc=%d\n",
__func__, gli->max_lba, gli->blk_len, rc);
diff --git a/drivers/scsi/cxlflash/vlun.c b/drivers/scsi/cxlflash/vlun.c
index 5deef57a7834..e7e9b2f2ad21 100644
--- a/drivers/scsi/cxlflash/vlun.c
+++ b/drivers/scsi/cxlflash/vlun.c
@@ -425,7 +425,6 @@ static int write_same16(struct scsi_device *sdev,
{
u8 *cmd_buf = NULL;
u8 *scsi_cmd = NULL;
- u8 *sense_buf = NULL;
int rc = 0;
int result = 0;
u64 offset = lba;
@@ -439,8 +438,7 @@ static int write_same16(struct scsi_device *sdev,
cmd_buf = kzalloc(CMD_BUFSIZE, GFP_KERNEL);
scsi_cmd = kzalloc(MAX_COMMAND_SIZE, GFP_KERNEL);
- sense_buf = kzalloc(SCSI_SENSE_BUFFERSIZE, GFP_KERNEL);
- if (unlikely(!cmd_buf || !scsi_cmd || !sense_buf)) {
+ if (unlikely(!cmd_buf || !scsi_cmd)) {
rc = -ENOMEM;
goto out;
}
@@ -456,7 +454,7 @@ static int write_same16(struct scsi_device *sdev,
/* Drop the ioctl read semahpore across lengthy call */
up_read(&cfg->ioctl_rwsem);
result = scsi_execute(sdev, scsi_cmd, DMA_TO_DEVICE, cmd_buf,
- CMD_BUFSIZE, sense_buf, NULL, to,
+ CMD_BUFSIZE, NULL, NULL, to,
CMD_RETRIES, 0, 0, NULL);
down_read(&cfg->ioctl_rwsem);
rc = check_state(cfg);
@@ -481,7 +479,6 @@ static int write_same16(struct scsi_device *sdev,
out:
kfree(cmd_buf);
kfree(scsi_cmd);
- kfree(sense_buf);
dev_dbg(dev, "%s: returning rc=%d\n", __func__, rc);
return rc;
}
--
2.17.0
[PATCH 0/6] block: Consolidate scsi sense buffer usage
This is a follow-up to commit f7068114d45e ("sr: pass down correctly
sized SCSI sense buffer") which further cleans up and removes needless
sense character array buffers and "struct request_sense" usage in favor
of the common "struct scsi_sense_hdr".
First, drop a bunch of unused sense buffers:
[PATCH 1/6] ide-cd: Drop unused sense buffers
[PATCH 2/6] scsi: cxlflash: Drop unused sense buffers
Next, split out struct scsi_sense_hdr:
[PATCH 3/6] block: Create scsi_sense.h for SCSI and ATAPI
Then move all request_sense usage to scsi_sense_hdr:
[PATCH 4/6] block: Consolidate scsi sense buffer usage
Finally add a build-time check to make sure we don't pass bad buffer sizes:
[PATCH 5/6] libata-scsi: Move sense buffers onto stack
[PATCH 6/6] scsi: Check sense buffer size at build time
-Kees
[PATCH 3/6] block: Create scsi_sense.h for SCSI and ATAPI
Both SCSI and ATAPI share the sense header. In preparation for using the
struct scsi_sense_hdr more widely, move this into a separate header and
move the helper function to scsi_ioctl.c which is linked with CONFIG_IDE
by way of CONFIG_BLK_SCSI_REQUEST.
Signed-off-by: Kees Cook
---
block/scsi_ioctl.c | 64 ++
drivers/scsi/scsi_common.c | 64 --
include/scsi/scsi_cmnd.h | 1 -
include/scsi/scsi_common.h | 32 +--
include/scsi/scsi_sense.h | 44 ++
5 files changed, 109 insertions(+), 96 deletions(-)
create mode 100644 include/scsi/scsi_sense.h
diff --git a/block/scsi_ioctl.c b/block/scsi_ioctl.c
index 60b471f8621b..87ff3cc7a364 100644
--- a/block/scsi_ioctl.c
+++ b/block/scsi_ioctl.c
@@ -728,6 +728,70 @@ void scsi_req_init(struct scsi_request *req)
}
EXPORT_SYMBOL(scsi_req_init);
+/**
+ * scsi_normalize_sense - normalize main elements from either fixed or
+ * descriptor sense data format into a common format.
+ *
+ * @sense_buffer: byte array containing sense data returned by device
+ * @sb_len:number of valid bytes in sense_buffer
+ * @sshdr: pointer to instance of structure that common
+ * elements are written to.
+ *
+ * Notes:
+ * The "main elements" from sense data are: response_code, sense_key,
+ * asc, ascq and additional_length (only for descriptor format).
+ *
+ * Typically this function can be called after a device has
+ * responded to a SCSI command with the CHECK_CONDITION status.
+ *
+ * Return value:
+ * true if valid sense data information found, else false;
+ */
+bool scsi_normalize_sense(const u8 *sense_buffer, int sb_len,
+ struct scsi_sense_hdr *sshdr)
+{
+ memset(sshdr, 0, sizeof(struct scsi_sense_hdr));
+
+ if (!sense_buffer || !sb_len)
+ return false;
+
+ sshdr->response_code = (sense_buffer[0] & 0x7f);
+
+ if (!scsi_sense_valid(sshdr))
+ return false;
+
+ if (sshdr->response_code >= 0x72) {
+ /*
+* descriptor format
+*/
+ if (sb_len > 1)
+ sshdr->sense_key = (sense_buffer[1] & 0xf);
+ if (sb_len > 2)
+ sshdr->asc = sense_buffer[2];
+ if (sb_len > 3)
+ sshdr->ascq = sense_buffer[3];
+ if (sb_len > 7)
+ sshdr->additional_length = sense_buffer[7];
+ } else {
+ /*
+* fixed format
+*/
+ if (sb_len > 2)
+ sshdr->sense_key = (sense_buffer[2] & 0xf);
+ if (sb_len > 7) {
+ sb_len = (sb_len < (sense_buffer[7] + 8)) ?
+sb_len : (sense_buffer[7] + 8);
+ if (sb_len > 12)
+ sshdr->asc = sense_buffer[12];
+ if (sb_len > 13)
+ sshdr->ascq = sense_buffer[13];
+ }
+ }
+
+ return true;
+}
+EXPORT_SYMBOL(scsi_normalize_sense);
+
static int __init blk_scsi_ioctl_init(void)
{
blk_set_cmd_filter_defaults(&blk_default_cmd_filter);
diff --git a/drivers/scsi/scsi_common.c b/drivers/scsi/scsi_common.c
index 90349498f686..8621a07cb967 100644
--- a/drivers/scsi/scsi_common.c
+++ b/drivers/scsi/scsi_common.c
@@ -116,70 +116,6 @@ void int_to_scsilun(u64 lun, struct scsi_lun *scsilun)
}
EXPORT_SYMBOL(int_to_scsilun);
-/**
- * scsi_normalize_sense - normalize main elements from either fixed or
- * descriptor sense data format into a common format.
- *
- * @sense_buffer: byte array containing sense data returned by device
- * @sb_len:number of valid bytes in sense_buffer
- * @sshdr: pointer to instance of structure that common
- * elements are written to.
- *
- * Notes:
- * The "main elements" from sense data are: response_code, sense_key,
- * asc, ascq and additional_length (only for descriptor format).
- *
- * Typically this function can be called after a device has
- * responded to a SCSI command with the CHECK_CONDITION status.
- *
- * Return value:
- * true if valid sense data information found, else false;
- */
-bool scsi_normalize_sense(const u8 *sense_buffer, int sb_len,
- struct scsi_sense_hdr *sshdr)
-{
- memset(sshdr, 0, sizeof(struct scsi_sense_hdr));
-
- if (!sense_buffer || !sb_len)
- return false;
-
- sshdr->response_code = (sense_buffer[0] & 0x7f);
-
- if (!scsi_sense_valid(sshdr))
- return false;
-
- if (sshdr->response_code >= 0x72) {
[PATCH 1/6] ide-cd: Drop unused sense buffers
This drops unused sense buffers from:
cdrom_eject()
cdrom_read_capacity()
cdrom_read_tocentry()
ide_cd_lockdoor()
ide_cd_read_toc()
Signed-off-by: Kees Cook
---
drivers/ide/ide-cd.c | 36 +++-
drivers/ide/ide-cd.h | 2 +-
drivers/ide/ide-cd_ioctl.c | 34 --
3 files changed, 28 insertions(+), 44 deletions(-)
diff --git a/drivers/ide/ide-cd.c b/drivers/ide/ide-cd.c
index 5a8e8e3c22cd..1d5b35312e33 100644
--- a/drivers/ide/ide-cd.c
+++ b/drivers/ide/ide-cd.c
@@ -890,8 +890,7 @@ int cdrom_check_status(ide_drive_t *drive, struct
request_sense *sense)
}
static int cdrom_read_capacity(ide_drive_t *drive, unsigned long *capacity,
- unsigned long *sectors_per_frame,
- struct request_sense *sense)
+ unsigned long *sectors_per_frame)
{
struct {
__be32 lba;
@@ -908,7 +907,7 @@ static int cdrom_read_capacity(ide_drive_t *drive, unsigned
long *capacity,
memset(cmd, 0, BLK_MAX_CDB);
cmd[0] = GPCMD_READ_CDVD_CAPACITY;
- stat = ide_cd_queue_pc(drive, cmd, 0, &capbuf, &len, sense, 0,
+ stat = ide_cd_queue_pc(drive, cmd, 0, &capbuf, &len, NULL, 0,
RQF_QUIET);
if (stat)
return stat;
@@ -944,8 +943,7 @@ static int cdrom_read_capacity(ide_drive_t *drive, unsigned
long *capacity,
}
static int cdrom_read_tocentry(ide_drive_t *drive, int trackno, int msf_flag,
- int format, char *buf, int buflen,
- struct request_sense *sense)
+ int format, char *buf, int buflen)
{
unsigned char cmd[BLK_MAX_CDB];
@@ -962,11 +960,11 @@ static int cdrom_read_tocentry(ide_drive_t *drive, int
trackno, int msf_flag,
if (msf_flag)
cmd[1] = 2;
- return ide_cd_queue_pc(drive, cmd, 0, buf, &buflen, sense, 0,
RQF_QUIET);
+ return ide_cd_queue_pc(drive, cmd, 0, buf, &buflen, NULL, 0, RQF_QUIET);
}
/* Try to read the entire TOC for the disk into our internal buffer. */
-int ide_cd_read_toc(ide_drive_t *drive, struct request_sense *sense)
+int ide_cd_read_toc(ide_drive_t *drive)
{
int stat, ntracks, i;
struct cdrom_info *info = drive->driver_data;
@@ -996,14 +994,13 @@ int ide_cd_read_toc(ide_drive_t *drive, struct
request_sense *sense)
* Check to see if the existing data is still valid. If it is,
* just return.
*/
- (void) cdrom_check_status(drive, sense);
+ (void) cdrom_check_status(drive, NULL);
if (drive->atapi_flags & IDE_AFLAG_TOC_VALID)
return 0;
/* try to get the total cdrom capacity and sector size */
- stat = cdrom_read_capacity(drive, &toc->capacity, §ors_per_frame,
- sense);
+ stat = cdrom_read_capacity(drive, &toc->capacity, §ors_per_frame);
if (stat)
toc->capacity = 0x1f;
@@ -1016,7 +1013,7 @@ int ide_cd_read_toc(ide_drive_t *drive, struct
request_sense *sense)
/* first read just the header, so we know how long the TOC is */
stat = cdrom_read_tocentry(drive, 0, 1, 0, (char *) &toc->hdr,
- sizeof(struct atapi_toc_header), sense);
+ sizeof(struct atapi_toc_header));
if (stat)
return stat;
@@ -1036,7 +1033,7 @@ int ide_cd_read_toc(ide_drive_t *drive, struct
request_sense *sense)
(char *)&toc->hdr,
sizeof(struct atapi_toc_header) +
(ntracks + 1) *
- sizeof(struct atapi_toc_entry), sense);
+ sizeof(struct atapi_toc_entry));
if (stat && toc->hdr.first_track > 1) {
/*
@@ -1056,8 +1053,7 @@ int ide_cd_read_toc(ide_drive_t *drive, struct
request_sense *sense)
(char *)&toc->hdr,
sizeof(struct atapi_toc_header) +
(ntracks + 1) *
- sizeof(struct atapi_toc_entry),
- sense);
+ sizeof(struct atapi_toc_entry));
if (stat)
return stat;
@@ -1094,7 +1090,7 @@ int ide_cd_read_toc(ide_drive_t *drive, struct
request_sense *sense)
if (toc->hdr.first_track != CDROM_LEADOUT) {
/* read the multisession information */
stat = cdrom_read_tocentry(drive, 0, 0, 1, (char *)&ms_tmp,
-
Re: [PATCH 3/6] block: Create scsi_sense.h for SCSI and ATAPI
On Tue, May 22, 2018 at 11:50 AM, Martin K. Petersen wrote: > > Christoph, > >> On Tue, May 22, 2018 at 11:15:09AM -0700, Kees Cook wrote: >>> Both SCSI and ATAPI share the sense header. In preparation for using the >>> struct scsi_sense_hdr more widely, move this into a separate header and >>> move the helper function to scsi_ioctl.c which is linked with CONFIG_IDE >>> by way of CONFIG_BLK_SCSI_REQUEST. >> >> Please keep the code where it is and just depend on SCSI on the legacy >> ide driver. No need to do gymnastics just for a legacy case. > > Yup, I agree. Oh, er, this was mainly done at Jens's request. Jens, can you advise? -Kees -- Kees Cook Pixel Security
Re: [PATCH 3/6] block: Create scsi_sense.h for SCSI and ATAPI
On Tue, May 22, 2018 at 12:16 PM, Jens Axboe wrote: > On 5/22/18 1:13 PM, Christoph Hellwig wrote: >> On Tue, May 22, 2018 at 01:09:41PM -0600, Jens Axboe wrote: >>> I think Martin and Christoph are objecting to moving the code to >>> block/scsi_ioctl.h. I don't care too much about where the code is, but >>> think it would be nice to have the definitions in a separate header. But >>> if they prefer just pulling in all of SCSI for it, well then I guess >>> it's pointless to move the header bits. Seems very heavy handed to me, >>> though. >> >> It might be heavy handed for the 3 remaining users of drivers/ide, > > Brutal :-) Heh. I noticed a similar sense buffer use in drivers/cdrom/cdrom.c too. Is this okay under the same considerations? diff --git a/drivers/block/Kconfig b/drivers/block/Kconfig index ad9b687a236a..220ff321c102 100644 --- a/drivers/block/Kconfig +++ b/drivers/block/Kconfig @@ -79,7 +79,7 @@ config GDROM tristate "SEGA Dreamcast GD-ROM drive" depends on SH_DREAMCAST select CDROM - select BLK_SCSI_REQUEST # only for the generic cdrom code + select SCSI help A standard SEGA Dreamcast comes with a modified CD ROM drive called a "GD-ROM" by SEGA to signify it is capable of reading special disks @@ -345,7 +345,7 @@ config CDROM_PKTCDVD tristate "Packet writing on CD/DVD media (DEPRECATED)" depends on !UML select CDROM - select BLK_SCSI_REQUEST + select SCSI help Note: This driver is deprecated and will be removed from the kernel in the near future! diff --git a/drivers/block/paride/Kconfig b/drivers/block/paride/Kconfig index f8bd6ef3605a..7fdfcc5eaca5 100644 --- a/drivers/block/paride/Kconfig +++ b/drivers/block/paride/Kconfig @@ -27,7 +27,7 @@ config PARIDE_PCD tristate "Parallel port ATAPI CD-ROMs" depends on PARIDE select CDROM - select BLK_SCSI_REQUEST # only for the generic cdrom code + select SCSI ---help--- This option enables the high-level driver for ATAPI CD-ROM devices connected through a parallel port. If you chose to build PARIDE >> but as long as that stuff just keeps working I'd rather worry about >> everyone else, and keep the scsi code where it belongs. > > Fine with me then, hopefully we can some day kill it off. I'll send a v2. I found a few other things to fix up (including the cdrom.c one). Thanks! -Kees -- Kees Cook Pixel Security
Re: [PATCH 3/6] block: Create scsi_sense.h for SCSI and ATAPI
On Tue, May 22, 2018 at 4:34 PM, Randy Dunlap wrote: > On 05/22/2018 04:31 PM, Kees Cook wrote: >> On Tue, May 22, 2018 at 12:16 PM, Jens Axboe wrote: >>> On 5/22/18 1:13 PM, Christoph Hellwig wrote: >>>> On Tue, May 22, 2018 at 01:09:41PM -0600, Jens Axboe wrote: >>>>> I think Martin and Christoph are objecting to moving the code to >>>>> block/scsi_ioctl.h. I don't care too much about where the code is, but >>>>> think it would be nice to have the definitions in a separate header. But >>>>> if they prefer just pulling in all of SCSI for it, well then I guess >>>>> it's pointless to move the header bits. Seems very heavy handed to me, >>>>> though. >>>> >>>> It might be heavy handed for the 3 remaining users of drivers/ide, >>> >>> Brutal :-) >> >> Heh. I noticed a similar sense buffer use in drivers/cdrom/cdrom.c >> too. Is this okay under the same considerations? > > No. Do not select an entire subsystem. Use depends on it instead. I looked at that first, but it seems it's designed for that. For example, "config ATA" already has a "select SCSI". It does look fishy, though, since "config SCSI" has a "depends" which would be ignored by "select". Luckily, all these uses already do a "depends on BLOCK" (directly or indirectly). -Kees -- Kees Cook Pixel Security
Re: [PATCH 3/6] block: Create scsi_sense.h for SCSI and ATAPI
On Tue, May 22, 2018 at 4:42 PM, Jens Axboe wrote: > On May 22, 2018, at 5:31 PM, Kees Cook wrote: >> >>> On Tue, May 22, 2018 at 12:16 PM, Jens Axboe wrote: >>>> On 5/22/18 1:13 PM, Christoph Hellwig wrote: >>>>> On Tue, May 22, 2018 at 01:09:41PM -0600, Jens Axboe wrote: >>>>> I think Martin and Christoph are objecting to moving the code to >>>>> block/scsi_ioctl.h. I don't care too much about where the code is, but >>>>> think it would be nice to have the definitions in a separate header. But >>>>> if they prefer just pulling in all of SCSI for it, well then I guess >>>>> it's pointless to move the header bits. Seems very heavy handed to me, >>>>> though. >>>> >>>> It might be heavy handed for the 3 remaining users of drivers/ide, >>> >>> Brutal :-) >> >> Heh. I noticed a similar sense buffer use in drivers/cdrom/cdrom.c >> too. Is this okay under the same considerations? > > This is going from somewhat crazy to pretty nuts, imho. I guess in practical > terms it doesn’t matter that much, since most folks would be using sr. I > still think a split would be vastly superior. Just keep the scsi code in > drivers/scsi/ and make it independently selectable. I had originally tied it to BLK_SCSI_REQUEST. Logically speaking, sense buffers are part of the request, and the CONFIG work is already done. This is roughly what I tried to do before, since scsi_ioctl.c is the only code pulled in for CONFIG_BLK_SCSI_REQUEST: $ git grep BLK_SCSI_REQUEST | grep Makefile block/Makefile:obj-$(CONFIG_BLK_SCSI_REQUEST) += scsi_ioctl.o Should I move to code to a new drivers/scsi/scsi_sense.c and add it to drivers/scsi/Makefile as: obj-$(CONFIG_BLK_SCSI_REQUEST)+= scsi_sense.o Every place I want to use the code is already covered by CONFIG_BLK_SCSI_REQUEST, so it seems like I just need to know where to put the .c file. :P -Kees -- Kees Cook Pixel Security
Re: [PATCH 3/6] block: Create scsi_sense.h for SCSI and ATAPI
On Wed, May 23, 2018 at 7:31 AM, Jens Axboe wrote: > On 5/23/18 8:25 AM, Christoph Hellwig wrote: >> On Wed, May 23, 2018 at 08:13:56AM -0600, Jens Axboe wrote: >>>> Should I move to code to a new drivers/scsi/scsi_sense.c and add it to >>>> drivers/scsi/Makefile as: >>>> >>>> obj-$(CONFIG_BLK_SCSI_REQUEST)+= scsi_sense.o >>>> >>>> Every place I want to use the code is already covered by >>>> CONFIG_BLK_SCSI_REQUEST, so it seems like I just need to know where to >>>> put the .c file. :P >>> >>> I think this is so much saner than a SCSI select or dependency, so I'll >>> have to disagree with Martin and Christoph. Just put it in drivers/scsi, >>> if it's the location they care about. >> >> I actually plan to remove CONFIG_BLK_SCSI_REQUEST in a merge window >> or two. The only users are scsi and the ide layer, (virtio_blk >> support has already been accidentally disabled for a while), and getting >> rid of it allows to to shrink and simply the scsi data structures. >> >> But if you want this for now lets keep scsi_sense.c in drivers/scsi >> but depend on CONFIG_BLK_SCSI_REQUEST, that is easy enough to fix up. > > It could be a stand-alone dependency, doesn't have to be BLK_SCSI_REQUEST. > BLA_SCSI_SENSE or something would do. I don't care too much about that, > mostly getting rid of the entire stack dependency. Aaand, I can't do this and leave it in drivers/scsi because of drivers/Makefile: obj-$(CONFIG_SCSI) += scsi/ So: this needs to live in block/ just like CONFIG_BLK_SCSI_REQUEST's scsi_ioctl.c. I will split it into CONFIG_BLK_SCSI_SENSE, but I'll still need to move the code from drivers/scsi/ to block/. Is this okay? -Kees -- Kees Cook Pixel Security
Re: [PATCH 6/6] scsi: Check sense buffer size at build time
On Wed, May 23, 2018 at 1:25 AM, Sergei Shtylyov
wrote:
> Hello!
>
> On 5/22/2018 9:15 PM, Kees Cook wrote:
>
>> To avoid introducing problems like those fixed in commit f7068114d45e
>> ("sr: pass down correctly sized SCSI sense buffer"), this creates a macro
>> wrapper for scsi_execute() that verifies the size of the sense buffer
>> similar to what was done for command string sizes in commit 3756f6401c30
>> ("exec: avoid gcc-8 warning for get_task_comm").
>>
>> Another solution could be to add another argument to scsi_execute(),
>> but this function already takes a lot of arguments and Jens was not fond
>> of that approach. As there was only a pair of dynamically allocated sense
>> buffers, this also moves those 96 bytes onto the stack to avoid triggering
>> the sizeof() check.
>>
>> Signed-off-by: Kees Cook
>> ---
>> drivers/scsi/scsi_lib.c| 6 +++---
>> include/scsi/scsi_device.h | 12 +++-
>> 2 files changed, 14 insertions(+), 4 deletions(-)
>>
> [...]
>>
>> diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
>> index 7ae177c8e399..1bb87b6c0ad2 100644
>> --- a/include/scsi/scsi_device.h
>> +++ b/include/scsi/scsi_device.h
>> @@ -426,11 +426,21 @@ extern const char *scsi_device_state_name(enum
>> scsi_device_state);
>> extern int scsi_is_sdev_device(const struct device *);
>> extern int scsi_is_target_device(const struct device *);
>> extern void scsi_sanitize_inquiry_string(unsigned char *s, int len);
>> -extern int scsi_execute(struct scsi_device *sdev, const unsigned char
>> *cmd,
>> +extern int __scsi_execute(struct scsi_device *sdev, const unsigned char
>> *cmd,
>> int data_direction, void *buffer, unsigned
>> bufflen,
>> unsigned char *sense, struct scsi_sense_hdr
>> *sshdr,
>> int timeout, int retries, u64 flags,
>> req_flags_t rq_flags, int *resid);
>> +/* Make sure any sense buffer is the correct size. */
>> +#define scsi_execute(sdev, cmd, data_direction, buffer, bufflen, sense,
>> \
>> +sshdr, timeout, retries, flags, rq_flags, resid) \
>> +({ \
>> + BUILD_BUG_ON((sense) != NULL && \
>> +sizeof(sense) != SCSI_SENSE_BUFFERSIZE); \
>
>
>This would only check the size of the 'sense' pointer, no?
Correct. Passing in a variable that was declared as:
char sense[SCSI_SENSE_BUFFERSIZE];
would pass this check. Dynamically allocated would not (and we don't
have any cases of that left after the prior patch in this series), and
it should cast improper casts.
If people don't like this bit of robustness vs complexity/limit, I'm
happy to leave it off the series.
-Kees
--
Kees Cook
Pixel Security
Re: [PATCH 3/6] block: Create scsi_sense.h for SCSI and ATAPI
On Wed, May 23, 2018 at 2:06 PM, Martin K. Petersen
wrote:
>
> Kees,
>
>> obj-$(CONFIG_SCSI) += scsi/
>>
>> So: this needs to live in block/ just like CONFIG_BLK_SCSI_REQUEST's
>> scsi_ioctl.c. I will split it into CONFIG_BLK_SCSI_SENSE, but I'll
>> still need to move the code from drivers/scsi/ to block/. Is this
>> okay?
>
> The reason this sucks is that scsi_normalize_sense() is an inherent core
> feature in the SCSI layer. Dealing with sense data for ioctls is just a
> fringe use case.
True, though I'm finding other robustness issues in the CDROM code.
They're probably all insane corner cases, but it seems like it'd be
nice to just fix them:
diff --git a/drivers/cdrom/cdrom.c b/drivers/cdrom/cdrom.c
index 3522d2cae1b6..7726c8618c30 100644
--- a/drivers/cdrom/cdrom.c
+++ b/drivers/cdrom/cdrom.c
@@ -,9 +2223,12 @@ static int cdrom_read_cdda_bpc(struct
cdrom_device_info *cdi, __u8 __user *ubuf,
blk_execute_rq(q, cdi->disk, rq, 0);
if (scsi_req(rq)->result) {
- struct request_sense *s = req->sense;
+ struct scsi_sense_hdr sshdr;
+
ret = -EIO;
- cdi->last_sense = s->sense_key;
+ scsi_normalize_sense(req->sense, req->sense_len,
+&sshdr);
+ cdi->last_sense = sshdr.sense_key;
}
if (blk_rq_unmap_user(bio))
This code wasn't checking req->sense_len, for example. It'll just get
stale data at worst case, but it's still ugly, especially since we
have a solution to do it right.
> I don't want to get too hung up on what goes where. But architecturally
> it really irks me to move a core piece of SCSI state machine
> functionality out of the subsystem to accommodate ioctl handling.
It looks like there is more in block/scsi_ioctl.c than just ioctl
handling, which is why I put the function in there originally.
Honestly, it's almost so small I could make it a static inline. :P
> I'm traveling today so I probably won't get a chance to look closely
> until tomorrow morning.
No worries; thanks for looking at it!
-Kees
--
Kees Cook
Pixel Security
Re: [PATCH 3/6] block: Create scsi_sense.h for SCSI and ATAPI
On Thu, May 24, 2018 at 1:00 AM, Christoph Hellwig wrote: > On Wed, May 23, 2018 at 03:14:19PM -0600, Jens Axboe wrote: >> Ugh, so that would necessitate a change there too. As I said before, >> I don't really care where it lives. I know the SCSI folks seem bothered >> by moving it, but in reality, it's not like this stuff will likely ever >> really change. Of the two choices (select entire SCSI stack, or just move >> this little bit), I know what I would consider the saner option... > > Oh well. How about something like this respin of Kees' series? > > http://git.infradead.org/users/hch/block.git/shortlog/refs/heads/sense-cleanup Does the CONFIG_PCMCIA in drivers/scsi/Makefile now get exposed in weird config cases? Otherwise, yeah, looks good to me. Thanks! -Kees -- Kees Cook Pixel Security
[PATCH 18/31] scsi/aic7xxx: Clean up timer usage
stat_timer only ever assigns the same function and data, so consolidate to
a setup_timer() call and drop everything else used to pass things around.
reset_timer is unused; remove it.
Cc: Hannes Reinecke
Cc: "James E.J. Bottomley"
Cc: "Martin K. Petersen"
Cc: linux-scsi@vger.kernel.org
Signed-off-by: Kees Cook
---
drivers/scsi/aic7xxx/aic79xx.h | 5 +
drivers/scsi/aic7xxx/aic79xx_core.c | 29 -
2 files changed, 9 insertions(+), 25 deletions(-)
diff --git a/drivers/scsi/aic7xxx/aic79xx.h b/drivers/scsi/aic7xxx/aic79xx.h
index d47b527b25dd..31f2bb9d7146 100644
--- a/drivers/scsi/aic7xxx/aic79xx.h
+++ b/drivers/scsi/aic7xxx/aic79xx.h
@@ -1046,8 +1046,6 @@ typedef enum {
typedef uint8_t ahd_mode_state;
-typedef void ahd_callback_t (void *);
-
struct ahd_completion
{
uint16_ttag;
@@ -1122,8 +1120,7 @@ struct ahd_softc {
/*
* Timer handles for timer driven callbacks.
*/
- ahd_timer_t reset_timer;
- ahd_timer_t stat_timer;
+ struct timer_list stat_timer;
/*
* Statistics.
diff --git a/drivers/scsi/aic7xxx/aic79xx_core.c
b/drivers/scsi/aic7xxx/aic79xx_core.c
index 95d8f25cbcca..a9fcc40eabcc 100644
--- a/drivers/scsi/aic7xxx/aic79xx_core.c
+++ b/drivers/scsi/aic7xxx/aic79xx_core.c
@@ -207,7 +207,7 @@ static void ahd_add_scb_to_free_list(struct
ahd_softc *ahd,
static u_int ahd_rem_wscb(struct ahd_softc *ahd, u_int scbid,
u_int prev, u_int next, u_int tid);
static voidahd_reset_current_bus(struct ahd_softc *ahd);
-static ahd_callback_t ahd_stat_timer;
+static voidahd_stat_timer(unsigned long data);
#ifdef AHD_DUMP_SEQ
static voidahd_dumpseq(struct ahd_softc *ahd);
#endif
@@ -6104,8 +6104,7 @@ ahd_alloc(void *platform_arg, char *name)
ahd->bugs = AHD_BUGNONE;
ahd->flags = AHD_SPCHK_ENB_A|AHD_RESET_BUS_A|AHD_TERM_ENB_A
| AHD_EXTENDED_TRANS_A|AHD_STPWLEVEL_A;
- ahd_timer_init(&ahd->reset_timer);
- ahd_timer_init(&ahd->stat_timer);
+ setup_timer(&ahd->stat_timer, ahd_stat_timer, (unsigned long)ahd);
ahd->int_coalescing_timer = AHD_INT_COALESCING_TIMER_DEFAULT;
ahd->int_coalescing_maxcmds = AHD_INT_COALESCING_MAXCMDS_DEFAULT;
ahd->int_coalescing_mincmds = AHD_INT_COALESCING_MINCMDS_DEFAULT;
@@ -6235,8 +6234,7 @@ ahd_shutdown(void *arg)
/*
* Stop periodic timer callbacks.
*/
- ahd_timer_stop(&ahd->reset_timer);
- ahd_timer_stop(&ahd->stat_timer);
+ del_timer_sync(&ahd->stat_timer);
/* This will reset most registers to 0, but not all */
ahd_reset(ahd, /*reinit*/FALSE);
@@ -7039,20 +7037,11 @@ static const char *termstat_strings[] = {
};
/* Timer Facilities
***/
-#define ahd_timer_init init_timer
-#define ahd_timer_stop del_timer_sync
-typedef void ahd_linux_callback_t (u_long);
-
static void
-ahd_timer_reset(ahd_timer_t *timer, int usec, ahd_callback_t *func, void *arg)
+ahd_timer_reset(ahd_timer_t *timer, int usec)
{
- struct ahd_softc *ahd;
-
- ahd = (struct ahd_softc *)arg;
del_timer(timer);
- timer->data = (u_long)arg;
timer->expires = jiffies + (usec * HZ)/100;
- timer->function = (ahd_linux_callback_t*)func;
add_timer(timer);
}
@@ -7279,8 +7268,7 @@ ahd_init(struct ahd_softc *ahd)
}
init_done:
ahd_restart(ahd);
- ahd_timer_reset(&ahd->stat_timer, AHD_STAT_UPDATE_US,
- ahd_stat_timer, ahd);
+ ahd_timer_reset(&ahd->stat_timer, AHD_STAT_UPDATE_US);
return (0);
}
@@ -8878,9 +8866,9 @@ ahd_reset_channel(struct ahd_softc *ahd, char channel,
int initiate_reset)
/ Statistics Processing
***/
static void
-ahd_stat_timer(void *arg)
+ahd_stat_timer(unsigned long data)
{
- struct ahd_softc *ahd = arg;
+ struct ahd_softc *ahd = (struct ahd_softc *)data;
u_long s;
int enint_coal;
@@ -8907,8 +8895,7 @@ ahd_stat_timer(void *arg)
ahd->cmdcmplt_bucket = (ahd->cmdcmplt_bucket+1) & (AHD_STAT_BUCKETS-1);
ahd->cmdcmplt_total -= ahd->cmdcmplt_counts[ahd->cmdcmplt_bucket];
ahd->cmdcmplt_counts[ahd->cmdcmplt_bucket] = 0;
- ahd_timer_reset(&ahd->stat_timer, AHD_STAT_UPDATE_US,
- ahd_stat_timer, ahd);
+ ahd_timer_reset(&ahd->stat_timer, AHD_STAT_UPDATE_US);
ahd_unlock(ahd, &s);
}
--
2.7.4
[PATCH 29/31] scsi/bnx2i: Initialize timer
There was a seemingly missing call to setup_timer() in one handler, so add setup_timer() here to remove the open-coded initialization. Cc: qlogic-storage-upstr...@qlogic.com Cc: "James E.J. Bottomley" Cc: "Martin K. Petersen" Cc: linux-scsi@vger.kernel.org Signed-off-by: Kees Cook --- drivers/scsi/bnx2i/bnx2i_iscsi.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/scsi/bnx2i/bnx2i_iscsi.c b/drivers/scsi/bnx2i/bnx2i_iscsi.c index bffc7e91b7e5..337139dadad0 100644 --- a/drivers/scsi/bnx2i/bnx2i_iscsi.c +++ b/drivers/scsi/bnx2i/bnx2i_iscsi.c @@ -1611,9 +1611,9 @@ static int bnx2i_conn_start(struct iscsi_cls_conn *cls_conn) * this should normally not sleep for a long time so it should * not disrupt the caller. */ + setup_timer(&bnx2i_conn->ep->ofld_timer, bnx2i_ep_ofld_timer, + (unsigned long) bnx2i_conn->ep); bnx2i_conn->ep->ofld_timer.expires = 1 * HZ + jiffies; - bnx2i_conn->ep->ofld_timer.function = bnx2i_ep_ofld_timer; - bnx2i_conn->ep->ofld_timer.data = (unsigned long) bnx2i_conn->ep; add_timer(&bnx2i_conn->ep->ofld_timer); /* update iSCSI context for this conn, wait for CNIC to complete */ wait_event_interruptible(bnx2i_conn->ep->ofld_wait, -- 2.7.4
[PATCH 19/31] timer: Remove open-coded casts for .data and .function
This standardizes the callback and data prototypes in several places that
perform casting, in an effort to remove more open-coded .data and
.function uses in favor of setup_timer().
Cc: Samuel Ortiz
Cc: Tyrel Datwyler
Cc: Benjamin Herrenschmidt
Cc: Paul Mackerras
Cc: Michael Ellerman
Cc: "James E.J. Bottomley"
Cc: "Martin K. Petersen"
Cc: net...@vger.kernel.org
Cc: linux-scsi@vger.kernel.org
Cc: linuxppc-...@lists.ozlabs.org
Signed-off-by: Kees Cook
---
drivers/net/irda/bfin_sir.c | 5 +++--
drivers/scsi/ibmvscsi/ibmvfc.c | 14 ++
drivers/scsi/ibmvscsi/ibmvscsi.c | 8
3 files changed, 13 insertions(+), 14 deletions(-)
diff --git a/drivers/net/irda/bfin_sir.c b/drivers/net/irda/bfin_sir.c
index 3151b580dbd6..c9413bd580a7 100644
--- a/drivers/net/irda/bfin_sir.c
+++ b/drivers/net/irda/bfin_sir.c
@@ -317,8 +317,9 @@ static void bfin_sir_dma_rx_chars(struct net_device *dev)
async_unwrap_char(dev, &self->stats, &self->rx_buff,
port->rx_dma_buf.buf[i]);
}
-void bfin_sir_rx_dma_timeout(struct net_device *dev)
+void bfin_sir_rx_dma_timeout(unsigned long data)
{
+ struct net_device *dev = (struct net_device *)data;
struct bfin_sir_self *self = netdev_priv(dev);
struct bfin_sir_port *port = self->sir_port;
int x_pos, pos;
@@ -406,7 +407,7 @@ static int bfin_sir_startup(struct bfin_sir_port *port,
struct net_device *dev)
enable_dma(port->rx_dma_channel);
port->rx_dma_timer.data = (unsigned long)(dev);
- port->rx_dma_timer.function = (void *)bfin_sir_rx_dma_timeout;
+ port->rx_dma_timer.function = bfin_sir_rx_dma_timeout;
#else
diff --git a/drivers/scsi/ibmvscsi/ibmvfc.c b/drivers/scsi/ibmvscsi/ibmvfc.c
index cc4e05be8d4a..1be20688dd1f 100644
--- a/drivers/scsi/ibmvscsi/ibmvfc.c
+++ b/drivers/scsi/ibmvscsi/ibmvfc.c
@@ -1393,8 +1393,9 @@ static int ibmvfc_map_sg_data(struct scsi_cmnd *scmd,
*
* Called when an internally generated command times out
**/
-static void ibmvfc_timeout(struct ibmvfc_event *evt)
+static void ibmvfc_timeout(unsigned long data)
{
+ struct ibmvfc_event *evt = (struct ibmvfc_event *)data;
struct ibmvfc_host *vhost = evt->vhost;
dev_err(vhost->dev, "Command timed out (%p). Resetting connection\n",
evt);
ibmvfc_reset_host(vhost);
@@ -1424,12 +1425,10 @@ static int ibmvfc_send_event(struct ibmvfc_event *evt,
BUG();
list_add_tail(&evt->queue, &vhost->sent);
- init_timer(&evt->timer);
+ setup_timer(&evt->timer, ibmvfc_timeout, (unsigned long)evt);
if (timeout) {
- evt->timer.data = (unsigned long) evt;
evt->timer.expires = jiffies + (timeout * HZ);
- evt->timer.function = (void (*)(unsigned long))ibmvfc_timeout;
add_timer(&evt->timer);
}
@@ -3696,8 +3695,9 @@ static void ibmvfc_tgt_adisc_cancel_done(struct
ibmvfc_event *evt)
* out, reset the CRQ. When the ADISC comes back as cancelled,
* log back into the target.
**/
-static void ibmvfc_adisc_timeout(struct ibmvfc_target *tgt)
+static void ibmvfc_adisc_timeout(unsigned long data)
{
+ struct ibmvfc_target *tgt = (struct ibmvfc_target *)data;
struct ibmvfc_host *vhost = tgt->vhost;
struct ibmvfc_event *evt;
struct ibmvfc_tmf *tmf;
@@ -3782,9 +3782,7 @@ static void ibmvfc_tgt_adisc(struct ibmvfc_target *tgt)
if (timer_pending(&tgt->timer))
mod_timer(&tgt->timer, jiffies + (IBMVFC_ADISC_TIMEOUT * HZ));
else {
- tgt->timer.data = (unsigned long) tgt;
tgt->timer.expires = jiffies + (IBMVFC_ADISC_TIMEOUT * HZ);
- tgt->timer.function = (void (*)(unsigned
long))ibmvfc_adisc_timeout;
add_timer(&tgt->timer);
}
@@ -3916,7 +3914,7 @@ static int ibmvfc_alloc_target(struct ibmvfc_host *vhost,
u64 scsi_id)
tgt->vhost = vhost;
tgt->need_login = 1;
tgt->cancel_key = vhost->task_set++;
- init_timer(&tgt->timer);
+ setup_timer(&tgt->timer, ibmvfc_adisc_timeout, (unsigned long)tgt);
kref_init(&tgt->kref);
ibmvfc_init_tgt(tgt, ibmvfc_tgt_implicit_logout);
spin_lock_irqsave(vhost->host->host_lock, flags);
diff --git a/drivers/scsi/ibmvscsi/ibmvscsi.c b/drivers/scsi/ibmvscsi/ibmvscsi.c
index da22b3665cb0..44ae85903a00 100644
--- a/drivers/scsi/ibmvscsi/ibmvscsi.c
+++ b/drivers/scsi/ibmvscsi/ibmvscsi.c
@@ -837,8 +837,9 @@ static void ibmvscsi_reset_host(struct ibmvscsi_host_data
*hostdata)
*
* Called when an internally generated command times out
*/
-static void ibmvscsi_timeout(struct srp_event_struct *evt_struct)
+static void ibmvscsi_timeout(unsigned long data)
{
+ struct srp_event_struct *evt_struct
Re: Buffer overflow in the mptctl_replace_fw() function in linux kernel MPT ioctl driver
On Thu, Aug 31, 2017 at 11:00 PM, Dison River wrote:
> Hi:
> Buffer overflow in the mptctl_replace_fw() function in linux kernel
> MPT ioctl driver.
>
> In mptctl_replace_fw function, kernel didn't check the size of
> "newFwSize" variable allows attackers to cause a denial of service via
> unspecified vectors that trigger copy_from_user function calls with
> improper length arguments.
Is there a specific path you see to trigger this?
>
>
> static int
> mptctl_replace_fw (unsigned long arg)
> {
> ..
> if (copy_from_user(&karg, uarg, sizeof(struct mpt_ioctl_replace_fw))) {
> printk(KERN_ERR MYNAM "%s@%d::mptctl_replace_fw - "
> "Unable to read in mpt_ioctl_replace_fw struct @ %p\n",
> __FILE__, __LINE__, uarg);
> return -EFAULT;
> }
>
> ..
>
> mpt_free_fw_memory(ioc);
This frees ioc->cached_fw.
>
> /* Allocate memory for the new FW image
> */
> newFwSize = ALIGN(karg.newImageSize, 4);
>
> mpt_alloc_fw_memory(ioc, newFwSize);
This allocates ioc->cached_fw to newFwSize.
> ..
>
> if (copy_from_user(ioc->cached_fw, uarg->newImage, newFwSize)) {
This copies newFwSize bytes into a buffer that is allocated to newFwSize bytes.
What am I missing? I see some games getting played in
mpt_alloc_fw_memory(), but they seem to be covered due to the earlier
call to mpt_free_fw_memory()...
-Kees
> ///--->newFwSize can control in userspace
> printk(MYIOC_s_ERR_FMT "%s@%d::mptctl_replace_fw - "
> "Unable to read in mpt_ioctl_replace_fw image "
> "@ %p\n", ioc->name, __FILE__, __LINE__, uarg);
> mpt_free_fw_memory(ioc);
> return -EFAULT;
> }
>
> ..
>
> return 0;
> }
-Kees
--
Kees Cook
Pixel Security
[PATCH v3 17/31] scsi: Define usercopy region in scsi_sense_cache slab cache
From: David Windsor
SCSI sense buffers, stored in struct scsi_cmnd.sense and therefore
contained in the scsi_sense_cache slab cache, need to be copied to/from
userspace.
cache object allocation:
drivers/scsi/scsi_lib.c:
scsi_select_sense_cache(...):
return ... ? scsi_sense_isadma_cache : scsi_sense_cache
scsi_alloc_sense_buffer(...):
return kmem_cache_alloc_node(scsi_select_sense_cache(), ...);
scsi_init_request(...):
...
cmd->sense_buffer = scsi_alloc_sense_buffer(...);
...
cmd->req.sense = cmd->sense_buffer
example usage trace:
block/scsi_ioctl.c:
(inline from sg_io)
blk_complete_sghdr_rq(...):
struct scsi_request *req = scsi_req(rq);
...
copy_to_user(..., req->sense, len)
scsi_cmd_ioctl(...):
sg_io(...);
In support of usercopy hardening, this patch defines a region in
the scsi_sense_cache slab cache in which userspace copy operations
are allowed.
This region is known as the slab cache's usercopy region. Slab
caches can now check that each copy operation involving cache-managed
memory falls entirely within the slab's usercopy region.
Signed-off-by: David Windsor
[kees: adjust commit log, provide usage trace]
Cc: "James E.J. Bottomley"
Cc: "Martin K. Petersen"
Cc: linux-scsi@vger.kernel.org
Signed-off-by: Kees Cook
---
drivers/scsi/scsi_lib.c | 9 +
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 9cf6a80fe297..88bfab251693 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -79,14 +79,15 @@ int scsi_init_sense_cache(struct Scsi_Host *shost)
if (shost->unchecked_isa_dma) {
scsi_sense_isadma_cache =
kmem_cache_create("scsi_sense_cache(DMA)",
- SCSI_SENSE_BUFFERSIZE, 0,
- SLAB_HWCACHE_ALIGN | SLAB_CACHE_DMA, NULL);
+ SCSI_SENSE_BUFFERSIZE, 0,
+ SLAB_HWCACHE_ALIGN | SLAB_CACHE_DMA, NULL);
if (!scsi_sense_isadma_cache)
ret = -ENOMEM;
} else {
scsi_sense_cache =
- kmem_cache_create("scsi_sense_cache",
- SCSI_SENSE_BUFFERSIZE, 0, SLAB_HWCACHE_ALIGN, NULL);
+ kmem_cache_create_usercopy("scsi_sense_cache",
+ SCSI_SENSE_BUFFERSIZE, 0, SLAB_HWCACHE_ALIGN,
+ 0, SCSI_SENSE_BUFFERSIZE, NULL);
if (!scsi_sense_cache)
ret = -ENOMEM;
}
--
2.7.4
[PATCH v2 19/31] timer: Remove open-coded casts for .data and .function
This standardizes the callback and data prototypes in several places that
perform casting, in an effort to remove more open-coded .data and
.function uses in favor of setup_timer().
Cc: Samuel Ortiz
Cc: Tyrel Datwyler
Cc: Benjamin Herrenschmidt
Cc: Paul Mackerras
Cc: Michael Ellerman
Cc: "James E.J. Bottomley"
Cc: "Martin K. Petersen"
Cc: net...@vger.kernel.org
Cc: linux-scsi@vger.kernel.org
Cc: linuxppc-...@lists.ozlabs.org
Signed-off-by: Kees Cook
Acked-by: Tyrel Datwyler # for ibmvscsi
---
drivers/scsi/ibmvscsi/ibmvfc.c | 14 ++
drivers/scsi/ibmvscsi/ibmvscsi.c| 8
drivers/staging/irda/drivers/bfin_sir.c | 5 +++--
3 files changed, 13 insertions(+), 14 deletions(-)
diff --git a/drivers/scsi/ibmvscsi/ibmvfc.c b/drivers/scsi/ibmvscsi/ibmvfc.c
index b491af31a5f8..66d51052caa1 100644
--- a/drivers/scsi/ibmvscsi/ibmvfc.c
+++ b/drivers/scsi/ibmvscsi/ibmvfc.c
@@ -1393,8 +1393,9 @@ static int ibmvfc_map_sg_data(struct scsi_cmnd *scmd,
*
* Called when an internally generated command times out
**/
-static void ibmvfc_timeout(struct ibmvfc_event *evt)
+static void ibmvfc_timeout(unsigned long data)
{
+ struct ibmvfc_event *evt = (struct ibmvfc_event *)data;
struct ibmvfc_host *vhost = evt->vhost;
dev_err(vhost->dev, "Command timed out (%p). Resetting connection\n",
evt);
ibmvfc_reset_host(vhost);
@@ -1424,12 +1425,10 @@ static int ibmvfc_send_event(struct ibmvfc_event *evt,
BUG();
list_add_tail(&evt->queue, &vhost->sent);
- init_timer(&evt->timer);
+ setup_timer(&evt->timer, ibmvfc_timeout, (unsigned long)evt);
if (timeout) {
- evt->timer.data = (unsigned long) evt;
evt->timer.expires = jiffies + (timeout * HZ);
- evt->timer.function = (void (*)(unsigned long))ibmvfc_timeout;
add_timer(&evt->timer);
}
@@ -3692,8 +3691,9 @@ static void ibmvfc_tgt_adisc_cancel_done(struct
ibmvfc_event *evt)
* out, reset the CRQ. When the ADISC comes back as cancelled,
* log back into the target.
**/
-static void ibmvfc_adisc_timeout(struct ibmvfc_target *tgt)
+static void ibmvfc_adisc_timeout(unsigned long data)
{
+ struct ibmvfc_target *tgt = (struct ibmvfc_target *)data;
struct ibmvfc_host *vhost = tgt->vhost;
struct ibmvfc_event *evt;
struct ibmvfc_tmf *tmf;
@@ -3778,9 +3778,7 @@ static void ibmvfc_tgt_adisc(struct ibmvfc_target *tgt)
if (timer_pending(&tgt->timer))
mod_timer(&tgt->timer, jiffies + (IBMVFC_ADISC_TIMEOUT * HZ));
else {
- tgt->timer.data = (unsigned long) tgt;
tgt->timer.expires = jiffies + (IBMVFC_ADISC_TIMEOUT * HZ);
- tgt->timer.function = (void (*)(unsigned
long))ibmvfc_adisc_timeout;
add_timer(&tgt->timer);
}
@@ -3912,7 +3910,7 @@ static int ibmvfc_alloc_target(struct ibmvfc_host *vhost,
u64 scsi_id)
tgt->vhost = vhost;
tgt->need_login = 1;
tgt->cancel_key = vhost->task_set++;
- init_timer(&tgt->timer);
+ setup_timer(&tgt->timer, ibmvfc_adisc_timeout, (unsigned long)tgt);
kref_init(&tgt->kref);
ibmvfc_init_tgt(tgt, ibmvfc_tgt_implicit_logout);
spin_lock_irqsave(vhost->host->host_lock, flags);
diff --git a/drivers/scsi/ibmvscsi/ibmvscsi.c b/drivers/scsi/ibmvscsi/ibmvscsi.c
index 7d156b161482..ffe917f02695 100644
--- a/drivers/scsi/ibmvscsi/ibmvscsi.c
+++ b/drivers/scsi/ibmvscsi/ibmvscsi.c
@@ -837,8 +837,9 @@ static void ibmvscsi_reset_host(struct ibmvscsi_host_data
*hostdata)
*
* Called when an internally generated command times out
*/
-static void ibmvscsi_timeout(struct srp_event_struct *evt_struct)
+static void ibmvscsi_timeout(unsigned long data)
{
+ struct srp_event_struct *evt_struct = (struct srp_event_struct *)data;
struct ibmvscsi_host_data *hostdata = evt_struct->hostdata;
dev_err(hostdata->dev, "Command timed out (%x). Resetting connection\n",
@@ -927,11 +928,10 @@ static int ibmvscsi_send_srp_event(struct
srp_event_struct *evt_struct,
*/
list_add_tail(&evt_struct->list, &hostdata->sent);
- init_timer(&evt_struct->timer);
+ setup_timer(&evt_struct->timer, ibmvscsi_timeout,
+ (unsigned long)evt_struct);
if (timeout) {
- evt_struct->timer.data = (unsigned long) evt_struct;
evt_struct->timer.expires = jiffies + (timeout * HZ);
- evt_struct->timer.function = (void (*)(unsigned
long))ibmvscsi_timeout;
add_timer(&evt_struct->timer);
}
diff --git a/drivers/staging/irda/drivers/bfin_sir.c
b/drivers/staging/irda/drivers/bfin_si
[PATCH v2 18/31] scsi/aic7xxx: Clean up timer usage
stat_timer only ever assigns the same function and data, so consolidate to
a setup_timer() call and drop everything else used to pass things around.
reset_timer is unused; remove it.
Cc: Hannes Reinecke
Cc: "James E.J. Bottomley"
Cc: "Martin K. Petersen"
Cc: linux-scsi@vger.kernel.org
Signed-off-by: Kees Cook
---
drivers/scsi/aic7xxx/aic79xx.h | 5 +
drivers/scsi/aic7xxx/aic79xx_core.c | 29 -
2 files changed, 9 insertions(+), 25 deletions(-)
diff --git a/drivers/scsi/aic7xxx/aic79xx.h b/drivers/scsi/aic7xxx/aic79xx.h
index d47b527b25dd..31f2bb9d7146 100644
--- a/drivers/scsi/aic7xxx/aic79xx.h
+++ b/drivers/scsi/aic7xxx/aic79xx.h
@@ -1046,8 +1046,6 @@ typedef enum {
typedef uint8_t ahd_mode_state;
-typedef void ahd_callback_t (void *);
-
struct ahd_completion
{
uint16_ttag;
@@ -1122,8 +1120,7 @@ struct ahd_softc {
/*
* Timer handles for timer driven callbacks.
*/
- ahd_timer_t reset_timer;
- ahd_timer_t stat_timer;
+ struct timer_list stat_timer;
/*
* Statistics.
diff --git a/drivers/scsi/aic7xxx/aic79xx_core.c
b/drivers/scsi/aic7xxx/aic79xx_core.c
index 95d8f25cbcca..a9fcc40eabcc 100644
--- a/drivers/scsi/aic7xxx/aic79xx_core.c
+++ b/drivers/scsi/aic7xxx/aic79xx_core.c
@@ -207,7 +207,7 @@ static void ahd_add_scb_to_free_list(struct
ahd_softc *ahd,
static u_int ahd_rem_wscb(struct ahd_softc *ahd, u_int scbid,
u_int prev, u_int next, u_int tid);
static voidahd_reset_current_bus(struct ahd_softc *ahd);
-static ahd_callback_t ahd_stat_timer;
+static voidahd_stat_timer(unsigned long data);
#ifdef AHD_DUMP_SEQ
static voidahd_dumpseq(struct ahd_softc *ahd);
#endif
@@ -6104,8 +6104,7 @@ ahd_alloc(void *platform_arg, char *name)
ahd->bugs = AHD_BUGNONE;
ahd->flags = AHD_SPCHK_ENB_A|AHD_RESET_BUS_A|AHD_TERM_ENB_A
| AHD_EXTENDED_TRANS_A|AHD_STPWLEVEL_A;
- ahd_timer_init(&ahd->reset_timer);
- ahd_timer_init(&ahd->stat_timer);
+ setup_timer(&ahd->stat_timer, ahd_stat_timer, (unsigned long)ahd);
ahd->int_coalescing_timer = AHD_INT_COALESCING_TIMER_DEFAULT;
ahd->int_coalescing_maxcmds = AHD_INT_COALESCING_MAXCMDS_DEFAULT;
ahd->int_coalescing_mincmds = AHD_INT_COALESCING_MINCMDS_DEFAULT;
@@ -6235,8 +6234,7 @@ ahd_shutdown(void *arg)
/*
* Stop periodic timer callbacks.
*/
- ahd_timer_stop(&ahd->reset_timer);
- ahd_timer_stop(&ahd->stat_timer);
+ del_timer_sync(&ahd->stat_timer);
/* This will reset most registers to 0, but not all */
ahd_reset(ahd, /*reinit*/FALSE);
@@ -7039,20 +7037,11 @@ static const char *termstat_strings[] = {
};
/* Timer Facilities
***/
-#define ahd_timer_init init_timer
-#define ahd_timer_stop del_timer_sync
-typedef void ahd_linux_callback_t (u_long);
-
static void
-ahd_timer_reset(ahd_timer_t *timer, int usec, ahd_callback_t *func, void *arg)
+ahd_timer_reset(ahd_timer_t *timer, int usec)
{
- struct ahd_softc *ahd;
-
- ahd = (struct ahd_softc *)arg;
del_timer(timer);
- timer->data = (u_long)arg;
timer->expires = jiffies + (usec * HZ)/100;
- timer->function = (ahd_linux_callback_t*)func;
add_timer(timer);
}
@@ -7279,8 +7268,7 @@ ahd_init(struct ahd_softc *ahd)
}
init_done:
ahd_restart(ahd);
- ahd_timer_reset(&ahd->stat_timer, AHD_STAT_UPDATE_US,
- ahd_stat_timer, ahd);
+ ahd_timer_reset(&ahd->stat_timer, AHD_STAT_UPDATE_US);
return (0);
}
@@ -8878,9 +8866,9 @@ ahd_reset_channel(struct ahd_softc *ahd, char channel,
int initiate_reset)
/ Statistics Processing
***/
static void
-ahd_stat_timer(void *arg)
+ahd_stat_timer(unsigned long data)
{
- struct ahd_softc *ahd = arg;
+ struct ahd_softc *ahd = (struct ahd_softc *)data;
u_long s;
int enint_coal;
@@ -8907,8 +8895,7 @@ ahd_stat_timer(void *arg)
ahd->cmdcmplt_bucket = (ahd->cmdcmplt_bucket+1) & (AHD_STAT_BUCKETS-1);
ahd->cmdcmplt_total -= ahd->cmdcmplt_counts[ahd->cmdcmplt_bucket];
ahd->cmdcmplt_counts[ahd->cmdcmplt_bucket] = 0;
- ahd_timer_reset(&ahd->stat_timer, AHD_STAT_UPDATE_US,
- ahd_stat_timer, ahd);
+ ahd_timer_reset(&ahd->stat_timer, AHD_STAT_UPDATE_US);
ahd_unlock(ahd, &s);
}
--
2.7.4
