diff mbox series

[SRU,F:linux-bluefield,v2,2/3] UBUNTU: SAUCE: mlx-bootctl: Fix potential buffer overflow

Message ID fc0387b89ad1ce637508913413997c6384f0246d.1623745760.git.shravankr@nvidia.com
State New
Headers show
Series Updates to mlx-bootctl | expand

Commit Message

Shravan Kumar Ramani June 15, 2021, 9:25 a.m. UTC 
BugLink: https://bugs.launchpad.net/bugs/1931981

Replace sprintf with snprintf to avoid buffer overflow.
Also, remove the redundant strlen usage since count is already
available in the _store functions

Signed-off-by: Shravan Kumar Ramani <shravankr@nvidia.com>
---
 drivers/platform/mellanox/mlx-bootctl.c | 83 ++++++++++++++-----------
 1 file changed, 46 insertions(+), 37 deletions(-)

Comments

Tim Gardner June 15, 2021, 12:13 p.m. UTC | #1 
In general, if you're worried about exclusion in the store functions, 
then you should also be worried about it in the read functions. It is 
likely arm_smcc_smc() is not thread safe.

You've also got some misuses of snprintf(). See inline. I've picked on 
the first few. They aren't fatal, but they also aren't right.

post_reset_wdog_show() does not use snprintf().

secure_boot_fuse_state_show() is unprotected against overflow.

Given the DRIVER_ATTR_RW macro interface you're using (with which I'm 
not familiar), I assume PAGE_SIZE is the size of the buffer given to 
these functions ?

On 6/15/21 3:25 AM, Shravan Kumar Ramani wrote:
> BugLink: https://bugs.launchpad.net/bugs/1931981
> 
> Replace sprintf with snprintf to avoid buffer overflow.
> Also, remove the redundant strlen usage since count is already
> available in the _store functions
> 
> Signed-off-by: Shravan Kumar Ramani <shravankr@nvidia.com>
> ---
>   drivers/platform/mellanox/mlx-bootctl.c | 83 ++++++++++++++-----------
>   1 file changed, 46 insertions(+), 37 deletions(-)
> 
> diff --git a/drivers/platform/mellanox/mlx-bootctl.c b/drivers/platform/mellanox/mlx-bootctl.c
> index 8ad38643ecb9..bf6e446dfa33 100644
> --- a/drivers/platform/mellanox/mlx-bootctl.c
> +++ b/drivers/platform/mellanox/mlx-bootctl.c
> @@ -174,8 +174,8 @@ static ssize_t post_reset_wdog_store(struct device_driver *drv,
>   static ssize_t reset_action_show(struct device_driver *drv,
>   				 char *buf)
>   {
> -	return sprintf(buf, "%s\n", reset_action_to_string(
> -			       smc_call0(MLNX_GET_RESET_ACTION)));
> +	return snprintf(buf, PAGE_SIZE, "%s\n", reset_action_to_string(
> +			smc_call0(MLNX_GET_RESET_ACTION)));
>   }
>   
>   static ssize_t reset_action_store(struct device_driver *drv,
> @@ -195,8 +195,8 @@ static ssize_t reset_action_store(struct device_driver *drv,
>   static ssize_t second_reset_action_show(struct device_driver *drv,
>   					char *buf)
>   {
> -	return sprintf(buf, "%s\n", reset_action_to_string(
> -			       smc_call0(MLNX_GET_SECOND_RESET_ACTION)));
> +	return snprintf(buf, PAGE_SIZE, "%s\n", reset_action_to_string(
> +			smc_call0(MLNX_GET_SECOND_RESET_ACTION)));
>   }
>   
>   static ssize_t second_reset_action_store(struct device_driver *drv,
> @@ -231,10 +231,11 @@ static ssize_t lifecycle_state_show(struct device_driver *drv,
>   
>   		lc_state &= SB_MODE_SECURE_MASK;
>   
> -		return sprintf(buf, "%s(test)\n", lifecycle_states[lc_state]);
> +		return snprintf(buf, PAGE_SIZE, "%s(test)\n",
> +				lifecycle_states[lc_state]);
>   	}
>   
> -	return sprintf(buf, "%s\n", lifecycle_states[lc_state]);
> +	return snprintf(buf, PAGE_SIZE, "%s\n", lifecycle_states[lc_state]);
>   }
>   
>   static ssize_t secure_boot_fuse_state_show(struct device_driver *drv,
> @@ -327,7 +328,7 @@ static ssize_t oob_mac_show(struct device_driver *drv, char *buf)
>   		mac_byte_ptr[0], mac_byte_ptr[1], mac_byte_ptr[2],
>   		mac_byte_ptr[3], mac_byte_ptr[4], mac_byte_ptr[5]);
>   
> -	return sprintf(buf, "%s\n", mac_str);
> +	return snprintf(buf, sizeof(mac_str), "%s", mac_str);

snprintf is really designed to prevent destination overflow. Using 
snprintf() this way really doesn't buy you anything, other then perhaps 
avoiding the use of strlen(mac_str) in the call to snprintf(). You also 
lose the last byte of mac_str, but thats OK in this case.

>   }
>   
>   static ssize_t oob_mac_store(struct device_driver *drv, const char *buf,
> @@ -379,7 +380,7 @@ static ssize_t opn_show(struct device_driver *drv, char *buf)
>   
>   	memcpy(opn, opn_data, MLNX_MFG_OPN_VAL_LEN);
>   
> -	return sprintf(buf, "%s", opn);
> +	return snprintf(buf, sizeof(opn), "%s", opn);

See comment above.

>   }
>   
>   static ssize_t opn_store(struct device_driver *drv, const char *buf,
> @@ -392,7 +393,7 @@ static ssize_t opn_store(struct device_driver *drv, const char *buf,
>   	if (count > MLNX_MFG_OPN_VAL_LEN)
>   		return -EINVAL;
>   
> -	memcpy(opn, buf, strlen(buf));
> +	memcpy(opn, buf, count);
>   
>   	mutex_lock(&mfg_ops_lock);
>   	for (word = 0; word < MLNX_MFG_VAL_WORD_CNT(OPN); word++) {
> @@ -427,7 +428,7 @@ static ssize_t sku_show(struct device_driver *drv, char *buf)
>   
>   	memcpy(sku, sku_data, MLNX_MFG_SKU_VAL_LEN);
>   
> -	return sprintf(buf, "%s", sku);
> +	return snprintf(buf, sizeof(sku), "%s", sku);

See comment above.

>   }
>   
>   static ssize_t sku_store(struct device_driver *drv, const char *buf,
> @@ -440,7 +441,7 @@ static ssize_t sku_store(struct device_driver *drv, const char *buf,
>   	if (count > MLNX_MFG_SKU_VAL_LEN)
>   		return -EINVAL;
>   
> -	memcpy(sku, buf, strlen(buf));
> +	memcpy(sku, buf, count);
>   
>   	mutex_lock(&mfg_ops_lock);
>   	for (word = 0; word < MLNX_MFG_VAL_WORD_CNT(SKU); word++) {
> @@ -475,7 +476,7 @@ static ssize_t modl_show(struct device_driver *drv, char *buf)
>   
>   	memcpy(modl, modl_data, MLNX_MFG_MODL_VAL_LEN);
>   
> -	return sprintf(buf, "%s", modl);
> +	return snprintf(buf, sizeof(modl), "%s", modl);

See comment above.

>   }
>   
>   static ssize_t modl_store(struct device_driver *drv, const char *buf,
> @@ -488,7 +489,7 @@ static ssize_t modl_store(struct device_driver *drv, const char *buf,
>   	if (count > MLNX_MFG_MODL_VAL_LEN)
>   		return -EINVAL;
>   
> -	memcpy(modl, buf, strlen(buf));
> +	memcpy(modl, buf, count);
>   
>   	mutex_lock(&mfg_ops_lock);
>   	for (word = 0; word < MLNX_MFG_VAL_WORD_CNT(MODL); word++) {
> @@ -523,7 +524,7 @@ static ssize_t sn_show(struct device_driver *drv, char *buf)
>   
>   	memcpy(sn, sn_data, MLNX_MFG_SN_VAL_LEN);
>   
> -	return sprintf(buf, "%s", sn);
> +	return snprintf(buf, sizeof(sn), "%s", sn);

See comment above.

>   }
>   
>   static ssize_t sn_store(struct device_driver *drv, const char *buf,
> @@ -536,7 +537,7 @@ static ssize_t sn_store(struct device_driver *drv, const char *buf,
>   	if (count > MLNX_MFG_SN_VAL_LEN)
>   		return -EINVAL;
>   
> -	memcpy(sn, buf, strlen(buf));
> +	memcpy(sn, buf, count);
>   
>   	mutex_lock(&mfg_ops_lock);
>   	for (word = 0; word < MLNX_MFG_VAL_WORD_CNT(SN); word++) {
> @@ -571,7 +572,7 @@ static ssize_t uuid_show(struct device_driver *drv, char *buf)
>   
>   	memcpy(uuid, uuid_data, MLNX_MFG_UUID_VAL_LEN);
>   
> -	return sprintf(buf, "%s", uuid);
> +	return snprintf(buf, sizeof(uuid), "%s", uuid);
>   }
>   
>   static ssize_t uuid_store(struct device_driver *drv, const char *buf,
> @@ -584,7 +585,7 @@ static ssize_t uuid_store(struct device_driver *drv, const char *buf,
>   	if (count > MLNX_MFG_UUID_VAL_LEN)
>   		return -EINVAL;
>   
> -	memcpy(uuid, buf, strlen(buf));
> +	memcpy(uuid, buf, count);
>   
>   	mutex_lock(&mfg_ops_lock);
>   	for (word = 0; word < MLNX_MFG_VAL_WORD_CNT(UUID); word++) {
> @@ -897,7 +898,7 @@ static char *rsh_log_get_reg_name(u64 opcode)
>   	return "unknown";
>   }
>   
> -static int rsh_log_show_crash(u64 hdr, char *buf)
> +static int rsh_log_show_crash(u64 hdr, char *buf, int size)
>   {
>   	int i, module, type, len, n = 0;
>   	u32 pc, syndrome, ec;
> @@ -913,17 +914,20 @@ static int rsh_log_show_crash(u64 hdr, char *buf)
>   	if (type == BF_RSH_LOG_TYPE_EXCEPTION) {
>   		syndrome = BF_RSH_LOG_HEADER_GET(SYNDROME, hdr);
>   		ec = syndrome >> AARCH64_ESR_ELX_EXCEPTION_CLASS_SHIFT;
> -		n = sprintf(p, " Exception(%s): syndrome = 0x%x%s\n",
> +		n = snprintf(p, size, " Exception(%s): syndrome = 0x%x%s\n",
>   			    rsh_log_mod[module], syndrome,
>   			    (ec == 0x24 || ec == 0x25) ? "(Data Abort)" :
>   			    (ec == 0x2f) ? "(SError)" : "");
>   	} else if (type == BF_RSH_LOG_TYPE_PANIC) {
>   		pc = BF_RSH_LOG_HEADER_GET(PC, hdr);
> -		n = sprintf(p, " PANIC(%s): PC = 0x%x\n", rsh_log_mod[module],
> -			    pc);
> +		n = snprintf(p, size,
> +			     " PANIC(%s): PC = 0x%x\n", rsh_log_mod[module],
> +			     pc);
>   	}
> -	if (n > 0)
> +	if (n > 0) {
>   		p += n;
> +		size -= n;
> +	}
>   
>   	/*
>   	 * Read the registers in a loop. 'len' is the total number of words in
> @@ -935,28 +939,31 @@ static int rsh_log_show_crash(u64 hdr, char *buf)
>   
>   		opcode = (opcode >> AARCH64_MRS_REG_SHIFT) &
>   			AARCH64_MRS_REG_MASK;
> -		n = sprintf(p, "   %-16s0x%llx\n", rsh_log_get_reg_name(opcode),
> -			    (unsigned long long)data);
> -		if (n > 0)
> +		n = snprintf(p, size,
> +			     "   %-16s0x%llx\n", rsh_log_get_reg_name(opcode),
> +			     (unsigned long long)data);
> +		if (n > 0) {
>   			p += n;
> +			size -= n;
> +		}
>   	}
>   
>   	return p - buf;
>   }
>   
> -static int rsh_log_format_msg(char *buf, const char *msg, ...)
> +static int rsh_log_format_msg(char *buf, int size, const char *msg, ...)
>   {
>   	va_list args;
>   	int len;
>   
>   	va_start(args, msg);
> -	len = vsprintf(buf, msg, args);
> +	len = vsnprintf(buf, size, msg, args);
>   	va_end(args);
>   
>   	return len;
>   }
>   
> -static int rsh_log_show_msg(u64 hdr, char *buf)
> +static int rsh_log_show_msg(u64 hdr, char *buf, int size)
>   {
>   	int has_arg = BF_RSH_LOG_HEADER_GET(HAS_ARG, hdr);
>   	int level = BF_RSH_LOG_HEADER_GET(LEVEL, hdr);
> @@ -987,13 +994,13 @@ static int rsh_log_show_msg(u64 hdr, char *buf)
>   	}
>   	*p = '\0';
>   	if (!has_arg) {
> -		len = sprintf(buf, " %s[%s]: %s\n", rsh_log_level[level],
> -			rsh_log_mod[module], msg);
> +		len = snprintf(buf, size, " %s[%s]: %s\n", rsh_log_level[level],
> +			       rsh_log_mod[module], msg);
>   	} else {
> -		len = sprintf(buf, " %s[%s]: ", rsh_log_level[level],
> -			rsh_log_mod[module]);
> -		len += rsh_log_format_msg(buf + len, msg, arg);
> -		len += sprintf(buf + len, "\n");
> +		len = snprintf(buf, size, " %s[%s]: ", rsh_log_level[level],
> +			       rsh_log_mod[module]);
> +		len += rsh_log_format_msg(buf + len, size - len, msg, arg);
> +		len += snprintf(buf + len, size - len, "\n");
>   	}
>   
>   	kfree(msg);
> @@ -1004,7 +1011,7 @@ static ssize_t rsh_log_show(struct device_driver *drv, char *buf)
>   {
>   	u64 hdr;
>   	char *p = buf;
> -	int i, n, rc, idx, type, len;
> +	int i, n, rc, idx, type, len, size = PAGE_SIZE;
>   
>   	if (!rsh_semaphore || !rsh_scratch_buf_ctl)
>   		return -EOPNOTSUPP;
> @@ -1032,12 +1039,14 @@ static ssize_t rsh_log_show(struct device_driver *drv, char *buf)
>   		switch (type) {
>   		case BF_RSH_LOG_TYPE_PANIC:
>   		case BF_RSH_LOG_TYPE_EXCEPTION:
> -			n = rsh_log_show_crash(hdr, p);
> +			n = rsh_log_show_crash(hdr, p, size);
>   			p += n;
> +			size -= n;
>   			break;
>   		case BF_RSH_LOG_TYPE_MSG:
> -			n = rsh_log_show_msg(hdr, p);
> +			n = rsh_log_show_msg(hdr, p, size);
>   			p += n;
> +			size -= n;
>   			break;
>   		default:
>   			/* Drain this message. */
>
diff mbox series

Patch

diff --git a/drivers/platform/mellanox/mlx-bootctl.c b/drivers/platform/mellanox/mlx-bootctl.c
index 8ad38643ecb9..bf6e446dfa33 100644
--- a/drivers/platform/mellanox/mlx-bootctl.c
+++ b/drivers/platform/mellanox/mlx-bootctl.c
@@ -174,8 +174,8 @@  static ssize_t post_reset_wdog_store(struct device_driver *drv,
 static ssize_t reset_action_show(struct device_driver *drv,
 				 char *buf)
 {
-	return sprintf(buf, "%s\n", reset_action_to_string(
-			       smc_call0(MLNX_GET_RESET_ACTION)));
+	return snprintf(buf, PAGE_SIZE, "%s\n", reset_action_to_string(
+			smc_call0(MLNX_GET_RESET_ACTION)));
 }
 
 static ssize_t reset_action_store(struct device_driver *drv,
@@ -195,8 +195,8 @@  static ssize_t reset_action_store(struct device_driver *drv,
 static ssize_t second_reset_action_show(struct device_driver *drv,
 					char *buf)
 {
-	return sprintf(buf, "%s\n", reset_action_to_string(
-			       smc_call0(MLNX_GET_SECOND_RESET_ACTION)));
+	return snprintf(buf, PAGE_SIZE, "%s\n", reset_action_to_string(
+			smc_call0(MLNX_GET_SECOND_RESET_ACTION)));
 }
 
 static ssize_t second_reset_action_store(struct device_driver *drv,
@@ -231,10 +231,11 @@  static ssize_t lifecycle_state_show(struct device_driver *drv,
 
 		lc_state &= SB_MODE_SECURE_MASK;
 
-		return sprintf(buf, "%s(test)\n", lifecycle_states[lc_state]);
+		return snprintf(buf, PAGE_SIZE, "%s(test)\n",
+				lifecycle_states[lc_state]);
 	}
 
-	return sprintf(buf, "%s\n", lifecycle_states[lc_state]);
+	return snprintf(buf, PAGE_SIZE, "%s\n", lifecycle_states[lc_state]);
 }
 
 static ssize_t secure_boot_fuse_state_show(struct device_driver *drv,
@@ -327,7 +328,7 @@  static ssize_t oob_mac_show(struct device_driver *drv, char *buf)
 		mac_byte_ptr[0], mac_byte_ptr[1], mac_byte_ptr[2],
 		mac_byte_ptr[3], mac_byte_ptr[4], mac_byte_ptr[5]);
 
-	return sprintf(buf, "%s\n", mac_str);
+	return snprintf(buf, sizeof(mac_str), "%s", mac_str);
 }
 
 static ssize_t oob_mac_store(struct device_driver *drv, const char *buf,
@@ -379,7 +380,7 @@  static ssize_t opn_show(struct device_driver *drv, char *buf)
 
 	memcpy(opn, opn_data, MLNX_MFG_OPN_VAL_LEN);
 
-	return sprintf(buf, "%s", opn);
+	return snprintf(buf, sizeof(opn), "%s", opn);
 }
 
 static ssize_t opn_store(struct device_driver *drv, const char *buf,
@@ -392,7 +393,7 @@  static ssize_t opn_store(struct device_driver *drv, const char *buf,
 	if (count > MLNX_MFG_OPN_VAL_LEN)
 		return -EINVAL;
 
-	memcpy(opn, buf, strlen(buf));
+	memcpy(opn, buf, count);
 
 	mutex_lock(&mfg_ops_lock);
 	for (word = 0; word < MLNX_MFG_VAL_WORD_CNT(OPN); word++) {
@@ -427,7 +428,7 @@  static ssize_t sku_show(struct device_driver *drv, char *buf)
 
 	memcpy(sku, sku_data, MLNX_MFG_SKU_VAL_LEN);
 
-	return sprintf(buf, "%s", sku);
+	return snprintf(buf, sizeof(sku), "%s", sku);
 }
 
 static ssize_t sku_store(struct device_driver *drv, const char *buf,
@@ -440,7 +441,7 @@  static ssize_t sku_store(struct device_driver *drv, const char *buf,
 	if (count > MLNX_MFG_SKU_VAL_LEN)
 		return -EINVAL;
 
-	memcpy(sku, buf, strlen(buf));
+	memcpy(sku, buf, count);
 
 	mutex_lock(&mfg_ops_lock);
 	for (word = 0; word < MLNX_MFG_VAL_WORD_CNT(SKU); word++) {
@@ -475,7 +476,7 @@  static ssize_t modl_show(struct device_driver *drv, char *buf)
 
 	memcpy(modl, modl_data, MLNX_MFG_MODL_VAL_LEN);
 
-	return sprintf(buf, "%s", modl);
+	return snprintf(buf, sizeof(modl), "%s", modl);
 }
 
 static ssize_t modl_store(struct device_driver *drv, const char *buf,
@@ -488,7 +489,7 @@  static ssize_t modl_store(struct device_driver *drv, const char *buf,
 	if (count > MLNX_MFG_MODL_VAL_LEN)
 		return -EINVAL;
 
-	memcpy(modl, buf, strlen(buf));
+	memcpy(modl, buf, count);
 
 	mutex_lock(&mfg_ops_lock);
 	for (word = 0; word < MLNX_MFG_VAL_WORD_CNT(MODL); word++) {
@@ -523,7 +524,7 @@  static ssize_t sn_show(struct device_driver *drv, char *buf)
 
 	memcpy(sn, sn_data, MLNX_MFG_SN_VAL_LEN);
 
-	return sprintf(buf, "%s", sn);
+	return snprintf(buf, sizeof(sn), "%s", sn);
 }
 
 static ssize_t sn_store(struct device_driver *drv, const char *buf,
@@ -536,7 +537,7 @@  static ssize_t sn_store(struct device_driver *drv, const char *buf,
 	if (count > MLNX_MFG_SN_VAL_LEN)
 		return -EINVAL;
 
-	memcpy(sn, buf, strlen(buf));
+	memcpy(sn, buf, count);
 
 	mutex_lock(&mfg_ops_lock);
 	for (word = 0; word < MLNX_MFG_VAL_WORD_CNT(SN); word++) {
@@ -571,7 +572,7 @@  static ssize_t uuid_show(struct device_driver *drv, char *buf)
 
 	memcpy(uuid, uuid_data, MLNX_MFG_UUID_VAL_LEN);
 
-	return sprintf(buf, "%s", uuid);
+	return snprintf(buf, sizeof(uuid), "%s", uuid);
 }
 
 static ssize_t uuid_store(struct device_driver *drv, const char *buf,
@@ -584,7 +585,7 @@  static ssize_t uuid_store(struct device_driver *drv, const char *buf,
 	if (count > MLNX_MFG_UUID_VAL_LEN)
 		return -EINVAL;
 
-	memcpy(uuid, buf, strlen(buf));
+	memcpy(uuid, buf, count);
 
 	mutex_lock(&mfg_ops_lock);
 	for (word = 0; word < MLNX_MFG_VAL_WORD_CNT(UUID); word++) {
@@ -897,7 +898,7 @@  static char *rsh_log_get_reg_name(u64 opcode)
 	return "unknown";
 }
 
-static int rsh_log_show_crash(u64 hdr, char *buf)
+static int rsh_log_show_crash(u64 hdr, char *buf, int size)
 {
 	int i, module, type, len, n = 0;
 	u32 pc, syndrome, ec;
@@ -913,17 +914,20 @@  static int rsh_log_show_crash(u64 hdr, char *buf)
 	if (type == BF_RSH_LOG_TYPE_EXCEPTION) {
 		syndrome = BF_RSH_LOG_HEADER_GET(SYNDROME, hdr);
 		ec = syndrome >> AARCH64_ESR_ELX_EXCEPTION_CLASS_SHIFT;
-		n = sprintf(p, " Exception(%s): syndrome = 0x%x%s\n",
+		n = snprintf(p, size, " Exception(%s): syndrome = 0x%x%s\n",
 			    rsh_log_mod[module], syndrome,
 			    (ec == 0x24 || ec == 0x25) ? "(Data Abort)" :
 			    (ec == 0x2f) ? "(SError)" : "");
 	} else if (type == BF_RSH_LOG_TYPE_PANIC) {
 		pc = BF_RSH_LOG_HEADER_GET(PC, hdr);
-		n = sprintf(p, " PANIC(%s): PC = 0x%x\n", rsh_log_mod[module],
-			    pc);
+		n = snprintf(p, size,
+			     " PANIC(%s): PC = 0x%x\n", rsh_log_mod[module],
+			     pc);
 	}
-	if (n > 0)
+	if (n > 0) {
 		p += n;
+		size -= n;
+	}
 
 	/*
 	 * Read the registers in a loop. 'len' is the total number of words in
@@ -935,28 +939,31 @@  static int rsh_log_show_crash(u64 hdr, char *buf)
 
 		opcode = (opcode >> AARCH64_MRS_REG_SHIFT) &
 			AARCH64_MRS_REG_MASK;
-		n = sprintf(p, "   %-16s0x%llx\n", rsh_log_get_reg_name(opcode),
-			    (unsigned long long)data);
-		if (n > 0)
+		n = snprintf(p, size,
+			     "   %-16s0x%llx\n", rsh_log_get_reg_name(opcode),
+			     (unsigned long long)data);
+		if (n > 0) {
 			p += n;
+			size -= n;
+		}
 	}
 
 	return p - buf;
 }
 
-static int rsh_log_format_msg(char *buf, const char *msg, ...)
+static int rsh_log_format_msg(char *buf, int size, const char *msg, ...)
 {
 	va_list args;
 	int len;
 
 	va_start(args, msg);
-	len = vsprintf(buf, msg, args);
+	len = vsnprintf(buf, size, msg, args);
 	va_end(args);
 
 	return len;
 }
 
-static int rsh_log_show_msg(u64 hdr, char *buf)
+static int rsh_log_show_msg(u64 hdr, char *buf, int size)
 {
 	int has_arg = BF_RSH_LOG_HEADER_GET(HAS_ARG, hdr);
 	int level = BF_RSH_LOG_HEADER_GET(LEVEL, hdr);
@@ -987,13 +994,13 @@  static int rsh_log_show_msg(u64 hdr, char *buf)
 	}
 	*p = '\0';
 	if (!has_arg) {
-		len = sprintf(buf, " %s[%s]: %s\n", rsh_log_level[level],
-			rsh_log_mod[module], msg);
+		len = snprintf(buf, size, " %s[%s]: %s\n", rsh_log_level[level],
+			       rsh_log_mod[module], msg);
 	} else {
-		len = sprintf(buf, " %s[%s]: ", rsh_log_level[level],
-			rsh_log_mod[module]);
-		len += rsh_log_format_msg(buf + len, msg, arg);
-		len += sprintf(buf + len, "\n");
+		len = snprintf(buf, size, " %s[%s]: ", rsh_log_level[level],
+			       rsh_log_mod[module]);
+		len += rsh_log_format_msg(buf + len, size - len, msg, arg);
+		len += snprintf(buf + len, size - len, "\n");
 	}
 
 	kfree(msg);
@@ -1004,7 +1011,7 @@  static ssize_t rsh_log_show(struct device_driver *drv, char *buf)
 {
 	u64 hdr;
 	char *p = buf;
-	int i, n, rc, idx, type, len;
+	int i, n, rc, idx, type, len, size = PAGE_SIZE;
 
 	if (!rsh_semaphore || !rsh_scratch_buf_ctl)
 		return -EOPNOTSUPP;
@@ -1032,12 +1039,14 @@  static ssize_t rsh_log_show(struct device_driver *drv, char *buf)
 		switch (type) {
 		case BF_RSH_LOG_TYPE_PANIC:
 		case BF_RSH_LOG_TYPE_EXCEPTION:
-			n = rsh_log_show_crash(hdr, p);
+			n = rsh_log_show_crash(hdr, p, size);
 			p += n;
+			size -= n;
 			break;
 		case BF_RSH_LOG_TYPE_MSG:
-			n = rsh_log_show_msg(hdr, p);
+			n = rsh_log_show_msg(hdr, p, size);
 			p += n;
+			size -= n;
 			break;
 		default:
 			/* Drain this message. */