[RFC] scsi: Avoid sign extension when setting command result bytes, was Re: [PATCH v5 00/13] scsi: core: fix uninit-value access of variable sshdr

Commit Message

Finn Thain Oct. 18, 2019, 11:17 p.m. UTC

On Fri, 18 Oct 2019, Martin K. Petersen wrote:

>
> (Sorry, zhengbin, you opened a can of worms. This is some of our oldest
> and most arcane code in SCSI)
>

A call to set_host_byte(cmd, x) or set_msg_byte(cmd, x) when x & 0x80 is 
set clobbers the most significant bytes in cmd->result.

Avoid this implicit sign extension when shifting bits by using an
unsigned formal parameter.

This is a theoretical bug, found by inspection, so I'm sending an untested 
RFC patch.

I didn't try to find possible callers that might pass a negative parameter
and neither did I check whether the clobber might be intentional...

Patch

diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
index 76ed5e4acd38..ae814fa68bb8 100644
--- a/include/scsi/scsi_cmnd.h
+++ b/include/scsi/scsi_cmnd.h
@@ -306,17 +306,17 @@  static inline struct scsi_data_buffer *scsi_prot(struct scsi_cmnd *cmd)
 #define scsi_for_each_prot_sg(cmd, sg, nseg, __i)		\
 	for_each_sg(scsi_prot_sglist(cmd), sg, nseg, __i)
 
-static inline void set_msg_byte(struct scsi_cmnd *cmd, char status)
+static inline void set_msg_byte(struct scsi_cmnd *cmd, unsigned char status)
 {
 	cmd->result = (cmd->result & 0xffff00ff) | (status << 8);
 }
 
-static inline void set_host_byte(struct scsi_cmnd *cmd, char status)
+static inline void set_host_byte(struct scsi_cmnd *cmd, unsigned char status)
 {
 	cmd->result = (cmd->result & 0xff00ffff) | (status << 16);
 }
 
-static inline void set_driver_byte(struct scsi_cmnd *cmd, char status)
+static inline void set_driver_byte(struct scsi_cmnd *cmd, unsigned char status)
 {
 	cmd->result = (cmd->result & 0x00ffffff) | (status << 24);
 }