[PATCH v2 0/6] Ensure the copied buf is NUL terminated

[PATCH v2 0/6] Ensure the copied buf is NUL terminated

[Bui Quang Minh]

Hi everyone,

I found that some drivers contains an out-of-bound read pattern like this

	kern_buf = memdup_user(user_buf, count);
	...
	sscanf(kern_buf, ...);

The sscanf can be replaced by some other string-related functions. This
pattern can lead to out-of-bound read of kern_buf in string-related
functions.

This series fix the above issue by replacing memdup_user with
memdup_user_nul.

Thanks,
Quang Minh.

To: Jesse Brandeburg <jesse.brandeburg@intel.com>
To: Tony Nguyen <anthony.l.nguyen@intel.com>
To: David S. Miller <davem@davemloft.net>
To: Eric Dumazet <edumazet@google.com>
To: Jakub Kicinski <kuba@kernel.org>
To: Paolo Abeni <pabeni@redhat.com>
To: Paul M Stillwell Jr <paul.m.stillwell.jr@intel.com>
To: Rasesh Mody <rmody@marvell.com>
To: Sudarsana Kalluru <skalluru@marvell.com>
To: GR-Linux-NIC-Dev@marvell.com
To: Anil Gurumurthy <anil.gurumurthy@qlogic.com>
To: Sudarsana Kalluru <sudarsana.kalluru@qlogic.com>
To: James E.J. Bottomley <James.Bottomley@HansenPartnership.com>
To: Martin K. Petersen <martin.petersen@oracle.com>
To: Fabian Frederick <fabf@skynet.be>
To: Saurav Kashyap <skashyap@marvell.com>
To: GR-QLogic-Storage-Upstream@marvell.com
To: Nilesh Javali <nilesh.javali@cavium.com>
To: Arun Easi <arun.easi@cavium.com>
To: Manish Rangankar <manish.rangankar@cavium.com>
To: Vineeth Vijayan <vneethv@linux.ibm.com>
To: Peter Oberparleiter <oberpar@linux.ibm.com>
To: Heiko Carstens <hca@linux.ibm.com>
To: Vasily Gorbik <gor@linux.ibm.com>
To: Alexander Gordeev <agordeev@linux.ibm.com>
To: Christian Borntraeger <borntraeger@linux.ibm.com>
To: Sven Schnelle <svens@linux.ibm.com>
To: Dupuis, Chad <chad.dupuis@cavium.com>
To: Sunil Goutham <sgoutham@marvell.com>
To: Linu Cherian <lcherian@marvell.com>
To: Geetha sowjanya <gakula@marvell.com>
To: Jerin Jacob <jerinj@marvell.com>
To: hariprasad <hkelam@marvell.com>
To: Subbaraya Sundeep <sbhatta@marvell.com>
Cc: intel-wired-lan@lists.osuosl.org
Cc: netdev@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: linux-scsi@vger.kernel.org
Cc: Saurav Kashyap <saurav.kashyap@cavium.com>
Cc: linux-s390@vger.kernel.org
Cc: Jens Axboe <axboe@kernel.dk>
Signed-off-by: Bui Quang Minh <minhquangbui99@gmail.com>

Changes in v2:
- Patch 5: use memdup_user_nul instead
- Add patch 6
- Link to v1: https://lore.kernel.org/r/20240422-fix-oob-read-v1-0-e02854c30174@gmail.com

---
Bui Quang Minh (6):
      ice: ensure the copied buf is NUL terminated
      bna: ensure the copied buf is NUL terminated
      bfa: ensure the copied buf is NUL terminated
      qedf: ensure the copied buf is NUL terminated
      cio: ensure the copied buf is NUL terminated
      octeontx2-af: avoid off-by-one read from userspace

 drivers/net/ethernet/brocade/bna/bnad_debugfs.c         | 4 ++--
 drivers/net/ethernet/intel/ice/ice_debugfs.c            | 8 ++++----
 drivers/net/ethernet/marvell/octeontx2/af/rvu_debugfs.c | 4 +---
 drivers/s390/cio/cio_inject.c                           | 2 +-
 drivers/scsi/bfa/bfad_debugfs.c                         | 4 ++--
 drivers/scsi/qedf/qedf_debugfs.c                        | 2 +-
 6 files changed, 11 insertions(+), 13 deletions(-)
---
base-commit: ed30a4a51bb196781c8058073ea720133a65596f
change-id: 20240422-fix-oob-read-19ae7f8f3711

Best regards,
-- 
Bui Quang Minh <minhquangbui99@gmail.com>

[PATCH v2 1/6] ice: ensure the copied buf is NUL terminated

[Bui Quang Minh]

Currently, we allocate a count-sized kernel buffer and copy count bytes
from userspace to that buffer. Later, we use sscanf on this buffer but we
don't ensure that the string is terminated inside the buffer, this can lead
to OOB read when using sscanf. Fix this issue by using memdup_user_nul
instead of memdup_user.

Fixes: 96a9a9341cda ("ice: configure FW logging")
Fixes: 73671c3162c8 ("ice: enable FW logging")
Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
Signed-off-by: Bui Quang Minh <minhquangbui99@gmail.com>
---
 drivers/net/ethernet/intel/ice/ice_debugfs.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_debugfs.c b/drivers/net/ethernet/intel/ice/ice_debugfs.c
index d252d98218d0..9fc0fd95a13d 100644
--- a/drivers/net/ethernet/intel/ice/ice_debugfs.c
+++ b/drivers/net/ethernet/intel/ice/ice_debugfs.c
@@ -171,7 +171,7 @@ ice_debugfs_module_write(struct file *filp, const char __user *buf,
 	if (*ppos != 0 || count > 8)
 		return -EINVAL;
 
-	cmd_buf = memdup_user(buf, count);
+	cmd_buf = memdup_user_nul(buf, count);
 	if (IS_ERR(cmd_buf))
 		return PTR_ERR(cmd_buf);
 
@@ -257,7 +257,7 @@ ice_debugfs_nr_messages_write(struct file *filp, const char __user *buf,
 	if (*ppos != 0 || count > 4)
 		return -EINVAL;
 
-	cmd_buf = memdup_user(buf, count);
+	cmd_buf = memdup_user_nul(buf, count);
 	if (IS_ERR(cmd_buf))
 		return PTR_ERR(cmd_buf);
 
@@ -332,7 +332,7 @@ ice_debugfs_enable_write(struct file *filp, const char __user *buf,
 	if (*ppos != 0 || count > 2)
 		return -EINVAL;
 
-	cmd_buf = memdup_user(buf, count);
+	cmd_buf = memdup_user_nul(buf, count);
 	if (IS_ERR(cmd_buf))
 		return PTR_ERR(cmd_buf);
 
@@ -428,7 +428,7 @@ ice_debugfs_log_size_write(struct file *filp, const char __user *buf,
 	if (*ppos != 0 || count > 5)
 		return -EINVAL;
 
-	cmd_buf = memdup_user(buf, count);
+	cmd_buf = memdup_user_nul(buf, count);
 	if (IS_ERR(cmd_buf))
 		return PTR_ERR(cmd_buf);
 

-- 
2.34.1

[PATCH v2 2/6] bna: ensure the copied buf is NUL terminated

[Bui Quang Minh]

Currently, we allocate a nbytes-sized kernel buffer and copy nbytes from
userspace to that buffer. Later, we use sscanf on this buffer but we don't
ensure that the string is terminated inside the buffer, this can lead to
OOB read when using sscanf. Fix this issue by using memdup_user_nul
instead of memdup_user.

Fixes: 7afc5dbde091 ("bna: Add debugfs interface.")
Signed-off-by: Bui Quang Minh <minhquangbui99@gmail.com>
---
 drivers/net/ethernet/brocade/bna/bnad_debugfs.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/brocade/bna/bnad_debugfs.c b/drivers/net/ethernet/brocade/bna/bnad_debugfs.c
index 7246e13dd559..97291bfbeea5 100644
--- a/drivers/net/ethernet/brocade/bna/bnad_debugfs.c
+++ b/drivers/net/ethernet/brocade/bna/bnad_debugfs.c
@@ -312,7 +312,7 @@ bnad_debugfs_write_regrd(struct file *file, const char __user *buf,
 	void *kern_buf;
 
 	/* Copy the user space buf */
-	kern_buf = memdup_user(buf, nbytes);
+	kern_buf = memdup_user_nul(buf, nbytes);
 	if (IS_ERR(kern_buf))
 		return PTR_ERR(kern_buf);
 
@@ -372,7 +372,7 @@ bnad_debugfs_write_regwr(struct file *file, const char __user *buf,
 	void *kern_buf;
 
 	/* Copy the user space buf */
-	kern_buf = memdup_user(buf, nbytes);
+	kern_buf = memdup_user_nul(buf, nbytes);
 	if (IS_ERR(kern_buf))
 		return PTR_ERR(kern_buf);
 

-- 
2.34.1

[PATCH v2 3/6] bfa: ensure the copied buf is NUL terminated

[Bui Quang Minh]

Currently, we allocate a nbytes-sized kernel buffer and copy nbytes from
userspace to that buffer. Later, we use sscanf on this buffer but we don't
ensure that the string is terminated inside the buffer, this can lead to
OOB read when using sscanf. Fix this issue by using memdup_user_nul
instead of memdup_user.

Fixes: 9f30b674759b ("bfa: replace 2 kzalloc/copy_from_user by memdup_user")
Signed-off-by: Bui Quang Minh <minhquangbui99@gmail.com>
---
 drivers/scsi/bfa/bfad_debugfs.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/bfa/bfad_debugfs.c b/drivers/scsi/bfa/bfad_debugfs.c
index 52db147d9979..f6dd077d47c9 100644
--- a/drivers/scsi/bfa/bfad_debugfs.c
+++ b/drivers/scsi/bfa/bfad_debugfs.c
@@ -250,7 +250,7 @@ bfad_debugfs_write_regrd(struct file *file, const char __user *buf,
 	unsigned long flags;
 	void *kern_buf;
 
-	kern_buf = memdup_user(buf, nbytes);
+	kern_buf = memdup_user_nul(buf, nbytes);
 	if (IS_ERR(kern_buf))
 		return PTR_ERR(kern_buf);
 
@@ -317,7 +317,7 @@ bfad_debugfs_write_regwr(struct file *file, const char __user *buf,
 	unsigned long flags;
 	void *kern_buf;
 
-	kern_buf = memdup_user(buf, nbytes);
+	kern_buf = memdup_user_nul(buf, nbytes);
 	if (IS_ERR(kern_buf))
 		return PTR_ERR(kern_buf);
 

-- 
2.34.1

Re: [PATCH v2 3/6] bfa: ensure the copied buf is NUL terminated

[Martin K. Petersen]

Bui,

> Currently, we allocate a nbytes-sized kernel buffer and copy nbytes from
> userspace to that buffer. Later, we use sscanf on this buffer but we don't
> ensure that the string is terminated inside the buffer, this can lead to
> OOB read when using sscanf. Fix this issue by using memdup_user_nul
> instead of memdup_user.

Applied to 6.10/scsi-staging, thanks!

-- 
Martin K. Petersen	Oracle Linux Engineering

[PATCH v2 4/6] qedf: ensure the copied buf is NUL terminated

[Bui Quang Minh]

Currently, we allocate a count-sized kernel buffer and copy count from
userspace to that buffer. Later, we use kstrtouint on this buffer but we
don't ensure that the string is terminated inside the buffer, this can
lead to OOB read when using kstrtouint. Fix this issue by using
memdup_user_nul instead of memdup_user.

Fixes: 61d8658b4a43 ("scsi: qedf: Add QLogic FastLinQ offload FCoE driver framework.")
Signed-off-by: Bui Quang Minh <minhquangbui99@gmail.com>
---
 drivers/scsi/qedf/qedf_debugfs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/qedf/qedf_debugfs.c b/drivers/scsi/qedf/qedf_debugfs.c
index 451fd236bfd0..96174353e389 100644
--- a/drivers/scsi/qedf/qedf_debugfs.c
+++ b/drivers/scsi/qedf/qedf_debugfs.c
@@ -170,7 +170,7 @@ qedf_dbg_debug_cmd_write(struct file *filp, const char __user *buffer,
 	if (!count || *ppos)
 		return 0;
 
-	kern_buf = memdup_user(buffer, count);
+	kern_buf = memdup_user_nul(buffer, count);
 	if (IS_ERR(kern_buf))
 		return PTR_ERR(kern_buf);
 

-- 
2.34.1

Re: [PATCH v2 4/6] qedf: ensure the copied buf is NUL terminated

[Martin K. Petersen]

Bui,

> Currently, we allocate a count-sized kernel buffer and copy count from
> userspace to that buffer. Later, we use kstrtouint on this buffer but we
> don't ensure that the string is terminated inside the buffer, this can
> lead to OOB read when using kstrtouint. Fix this issue by using
> memdup_user_nul instead of memdup_user.

Applied to 6.10/scsi-staging, thanks!

-- 
Martin K. Petersen	Oracle Linux Engineering

[PATCH v2 5/6] cio: ensure the copied buf is NUL terminated

[Bui Quang Minh]

Currently, we allocate a lbuf-sized kernel buffer and copy lbuf from
userspace to that buffer. Later, we use scanf on this buffer but we don't
ensure that the string is terminated inside the buffer, this can lead to
OOB read when using scanf. Fix this issue by using memdup_user_nul instead.

Fixes: a4f17cc72671 ("s390/cio: add CRW inject functionality")
Signed-off-by: Bui Quang Minh <minhquangbui99@gmail.com>
---
 drivers/s390/cio/cio_inject.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/s390/cio/cio_inject.c b/drivers/s390/cio/cio_inject.c
index 8613fa937237..a2e771ebae8e 100644
--- a/drivers/s390/cio/cio_inject.c
+++ b/drivers/s390/cio/cio_inject.c
@@ -95,7 +95,7 @@ static ssize_t crw_inject_write(struct file *file, const char __user *buf,
 		return -EINVAL;
 	}
 
-	buffer = vmemdup_user(buf, lbuf);
+	buffer = memdup_user_nul(buf, lbuf);
 	if (IS_ERR(buffer))
 		return -ENOMEM;
 

-- 
2.34.1

Re: [PATCH v2 5/6] cio: ensure the copied buf is NUL terminated

[Heiko Carstens]

On Wed, Apr 24, 2024 at 09:44:22PM +0700, Bui Quang Minh wrote:
> Currently, we allocate a lbuf-sized kernel buffer and copy lbuf from
> userspace to that buffer. Later, we use scanf on this buffer but we don't
> ensure that the string is terminated inside the buffer, this can lead to
> OOB read when using scanf. Fix this issue by using memdup_user_nul instead.
> 
> Fixes: a4f17cc72671 ("s390/cio: add CRW inject functionality")
> Signed-off-by: Bui Quang Minh <minhquangbui99@gmail.com>
> ---
>  drivers/s390/cio/cio_inject.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Heiko Carstens <hca@linux.ibm.com>

Re: [PATCH v2 5/6] cio: ensure the copied buf is NUL terminated

[Alexander Gordeev]

On Wed, Apr 24, 2024 at 09:44:22PM +0700, Bui Quang Minh wrote:
> Currently, we allocate a lbuf-sized kernel buffer and copy lbuf from
> userspace to that buffer. Later, we use scanf on this buffer but we don't
> ensure that the string is terminated inside the buffer, this can lead to
> OOB read when using scanf. Fix this issue by using memdup_user_nul instead.
> 
> Fixes: a4f17cc72671 ("s390/cio: add CRW inject functionality")
> Signed-off-by: Bui Quang Minh <minhquangbui99@gmail.com>
> ---
>  drivers/s390/cio/cio_inject.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Applied, thanks!

Re: [PATCH v2 5/6] cio: ensure the copied buf is NUL terminated

[Alexander Gordeev]

On Wed, Apr 24, 2024 at 05:16:56PM +0200, Alexander Gordeev wrote:
> Applied, thanks!

Hi Jakub,

I just want to make sure you do not have plans to pull this patch
via the net tree, right? (I schedulled it for the s390 tree already).

Thanks!

Re: [PATCH v2 5/6] cio: ensure the copied buf is NUL terminated

[Jakub Kicinski]

On Fri, 26 Apr 2024 12:10:35 +0200 Alexander Gordeev wrote:
> On Wed, Apr 24, 2024 at 05:16:56PM +0200, Alexander Gordeev wrote:
> > Applied, thanks!  
> 
> Hi Jakub,
> 
> I just want to make sure you do not have plans to pull this patch
> via the net tree, right? (I schedulled it for the s390 tree already).

Yes, go for it. I picked 1, 2 and 6, no interest in the other 3 :)

[PATCH v2 6/6] octeontx2-af: avoid off-by-one read from userspace

[Bui Quang Minh]

We try to access count + 1 byte from userspace with memdup_user(buffer,
count + 1). However, the userspace only provides buffer of count bytes and
only these count bytes are verified to be okay to access. To ensure the
copied buffer is NUL terminated, we use memdup_user_nul instead.

Fixes: 3a2eb515d136 ("octeontx2-af: Fix an off by one in rvu_dbg_qsize_write()")
Signed-off-by: Bui Quang Minh <minhquangbui99@gmail.com>
---
 drivers/net/ethernet/marvell/octeontx2/af/rvu_debugfs.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/marvell/octeontx2/af/rvu_debugfs.c b/drivers/net/ethernet/marvell/octeontx2/af/rvu_debugfs.c
index 2500f5ba4f5a..881d704644fb 100644
--- a/drivers/net/ethernet/marvell/octeontx2/af/rvu_debugfs.c
+++ b/drivers/net/ethernet/marvell/octeontx2/af/rvu_debugfs.c
@@ -999,12 +999,10 @@ static ssize_t rvu_dbg_qsize_write(struct file *filp,
 	u16 pcifunc;
 	int ret, lf;
 
-	cmd_buf = memdup_user(buffer, count + 1);
+	cmd_buf = memdup_user_nul(buffer, count);
 	if (IS_ERR(cmd_buf))
 		return -ENOMEM;
 
-	cmd_buf[count] = '\0';
-
 	cmd_buf_tmp = strchr(cmd_buf, '\n');
 	if (cmd_buf_tmp) {
 		*cmd_buf_tmp = '\0';

-- 
2.34.1

Re: [PATCH v2 0/6] Ensure the copied buf is NUL terminated

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Wed, 24 Apr 2024 21:44:17 +0700 you wrote:
> Hi everyone,
> 
> I found that some drivers contains an out-of-bound read pattern like this
> 
> 	kern_buf = memdup_user(user_buf, count);
> 	...
> 	sscanf(kern_buf, ...);
> 
> [...]

Here is the summary with links:
  - [v2,1/6] ice: ensure the copied buf is NUL terminated
    https://git.kernel.org/netdev/net/c/666854ea9cad
  - [v2,2/6] bna: ensure the copied buf is NUL terminated
    https://git.kernel.org/netdev/net/c/8c34096c7fdf
  - [v2,3/6] bfa: ensure the copied buf is NUL terminated
    (no matching commit)
  - [v2,4/6] qedf: ensure the copied buf is NUL terminated
    (no matching commit)
  - [v2,5/6] cio: ensure the copied buf is NUL terminated
    (no matching commit)
  - [v2,6/6] octeontx2-af: avoid off-by-one read from userspace
    https://git.kernel.org/netdev/net/c/f299ee709fb4

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH v2 0/6] Ensure the copied buf is NUL terminated

[Martin K. Petersen]

On Wed, 24 Apr 2024 21:44:17 +0700, Bui Quang Minh wrote:

> I found that some drivers contains an out-of-bound read pattern like this
> 
> 	kern_buf = memdup_user(user_buf, count);
> 	...
> 	sscanf(kern_buf, ...);
> 
> The sscanf can be replaced by some other string-related functions. This
> pattern can lead to out-of-bound read of kern_buf in string-related
> functions.
> 
> [...]

Applied to 6.10/scsi-queue, thanks!

[3/6] bfa: ensure the copied buf is NUL terminated
      https://git.kernel.org/mkp/scsi/c/13d0cecb4626
[4/6] qedf: ensure the copied buf is NUL terminated
      https://git.kernel.org/mkp/scsi/c/d0184a375ee7

-- 
Martin K. Petersen	Oracle Linux Engineering