[PATCH] mount.cifs: fix buffer overrun in set_password

[PATCH] mount.cifs: fix buffer overrun in set_password

[David Disseldorp]

The existing (j > pass_length) check is insufficient to avoid dst buffer
overrun into the start of the adjacent struct parsed_mount_info field.
Check for overrun before writing to dst, and account for comma-expansion
and null-termination.

Bug: https://bugzilla.samba.org/show_bug.cgi?id=16044

Reported-by: Bruno Bierbaumer <bruno@bierbaumer.net>
Signed-off-by: David Disseldorp <ddiss@suse.de>
---
 mount.cifs.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/mount.cifs.c b/mount.cifs.c
index 1923913..d41ca6a 100644
--- a/mount.cifs.c
+++ b/mount.cifs.c
@@ -350,13 +350,13 @@ set_password(struct parsed_mount_info *parsed_info, const char *src,
 	unsigned int i = 0, j = 0;
 
 	while (src[i]) {
-		if (src[i] == ',')
-			dst[j++] = ',';
-		dst[j++] = src[i++];
-		if (j > pass_length) {
+		if (j + 2 >= pass_length) {
 			fprintf(stderr, "Converted password too long!\n");
 			return EX_USAGE;
 		}
+		if (src[i] == ',')
+			dst[j++] = ',';
+		dst[j++] = src[i++];
 	}
 	dst[j] = '\0';
 	if (is_pass2)
-- 
2.51.0

Re: [PATCH] mount.cifs: fix buffer overrun in set_password

[Henrique Carvalho]

Reviewed-by: Henrique Carvalho <henrique.carvalho@suse.com>

On Tue, Mar 31, 2026 at 11:09:11AM +1100, David Disseldorp wrote:
> The existing (j > pass_length) check is insufficient to avoid dst buffer
> overrun into the start of the adjacent struct parsed_mount_info field.
> Check for overrun before writing to dst, and account for comma-expansion
> and null-termination.
> 
> Bug: https://bugzilla.samba.org/show_bug.cgi?id=16044
> 
> Reported-by: Bruno Bierbaumer <bruno@bierbaumer.net>
> Signed-off-by: David Disseldorp <ddiss@suse.de>
> ---
>  mount.cifs.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/mount.cifs.c b/mount.cifs.c
> index 1923913..d41ca6a 100644
> --- a/mount.cifs.c
> +++ b/mount.cifs.c
> @@ -350,13 +350,13 @@ set_password(struct parsed_mount_info *parsed_info, const char *src,
>  	unsigned int i = 0, j = 0;
>  
>  	while (src[i]) {
> -		if (src[i] == ',')
> -			dst[j++] = ',';
> -		dst[j++] = src[i++];
> -		if (j > pass_length) {
> +		if (j + 2 >= pass_length) {
>  			fprintf(stderr, "Converted password too long!\n");
>  			return EX_USAGE;
>  		}
> +		if (src[i] == ',')
> +			dst[j++] = ',';
> +		dst[j++] = src[i++];
>  	}
>  	dst[j] = '\0';
>  	if (is_pass2)
> -- 
> 2.51.0
> 
> 

Re: [PATCH] mount.cifs: fix buffer overrun in set_password

[Steve French]

added RB from Henrique and updated
git.samba.org/data/git/cifs-utils.git and
https://github.com/smfrench/smb3-utils for-next

On Mon, Mar 30, 2026 at 7:10 PM David Disseldorp <ddiss@suse.de> wrote:
>
> The existing (j > pass_length) check is insufficient to avoid dst buffer
> overrun into the start of the adjacent struct parsed_mount_info field.
> Check for overrun before writing to dst, and account for comma-expansion
> and null-termination.
>
> Bug: https://bugzilla.samba.org/show_bug.cgi?id=16044
>
> Reported-by: Bruno Bierbaumer <bruno@bierbaumer.net>
> Signed-off-by: David Disseldorp <ddiss@suse.de>
> ---
>  mount.cifs.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/mount.cifs.c b/mount.cifs.c
> index 1923913..d41ca6a 100644
> --- a/mount.cifs.c
> +++ b/mount.cifs.c
> @@ -350,13 +350,13 @@ set_password(struct parsed_mount_info *parsed_info, const char *src,
>         unsigned int i = 0, j = 0;
>
>         while (src[i]) {
> -               if (src[i] == ',')
> -                       dst[j++] = ',';
> -               dst[j++] = src[i++];
> -               if (j > pass_length) {
> +               if (j + 2 >= pass_length) {
>                         fprintf(stderr, "Converted password too long!\n");
>                         return EX_USAGE;
>                 }
> +               if (src[i] == ',')
> +                       dst[j++] = ',';
> +               dst[j++] = src[i++];
>         }
>         dst[j] = '\0';
>         if (is_pass2)
> --
> 2.51.0
>


-- 
Thanks,

Steve

Re: [PATCH] mount.cifs: fix buffer overrun in set_password

[David Disseldorp]

On Tue, 31 Mar 2026 18:56:05 -0500, Steve French wrote:

> added RB from Henrique and updated
> git.samba.org/data/git/cifs-utils.git and
> https://github.com/smfrench/smb3-utils for-next

Thanks Steve. FWIW, the reviewed-by tag has an incorrect email address:
  Reviewed-by: Henrique Carvalho <[2]henrique.carvalho@suse.com>
Should be
  Reviewed-by: Henrique Carvalho <henrique.carvalho@suse.com>

I'll close the corresponding PR at
https://github.com/smfrench/smb3-utils/pull/16

Cheers, David

> On Mon, Mar 30, 2026 at 7:10 PM David Disseldorp <ddiss@suse.de> wrote:
> >
> > The existing (j > pass_length) check is insufficient to avoid dst buffer
> > overrun into the start of the adjacent struct parsed_mount_info field.
> > Check for overrun before writing to dst, and account for comma-expansion
> > and null-termination.
> >
> > Bug: https://bugzilla.samba.org/show_bug.cgi?id=16044
> >
> > Reported-by: Bruno Bierbaumer <bruno@bierbaumer.net>
> > Signed-off-by: David Disseldorp <ddiss@suse.de>
> > ---
> >  mount.cifs.c | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/mount.cifs.c b/mount.cifs.c
> > index 1923913..d41ca6a 100644
> > --- a/mount.cifs.c
> > +++ b/mount.cifs.c
> > @@ -350,13 +350,13 @@ set_password(struct parsed_mount_info *parsed_info, const char *src,
> >         unsigned int i = 0, j = 0;
> >
> >         while (src[i]) {
> > -               if (src[i] == ',')
> > -                       dst[j++] = ',';
> > -               dst[j++] = src[i++];
> > -               if (j > pass_length) {
> > +               if (j + 2 >= pass_length) {
> >                         fprintf(stderr, "Converted password too long!\n");
> >                         return EX_USAGE;
> >                 }
> > +               if (src[i] == ',')
> > +                       dst[j++] = ',';
> > +               dst[j++] = src[i++];
> >         }
> >         dst[j] = '\0';
> >         if (is_pass2)
> > --
> > 2.51.0
> >  
> 
> 

Re: [PATCH] mount.cifs: fix buffer overrun in set_password

[Steve French]

Good catch.  Fixed the RB tag

On Tue, Mar 31, 2026 at 7:58 PM David Disseldorp <ddiss@suse.de> wrote:
>
> On Tue, 31 Mar 2026 18:56:05 -0500, Steve French wrote:
>
> > added RB from Henrique and updated
> > git.samba.org/data/git/cifs-utils.git and
> > https://github.com/smfrench/smb3-utils for-next
>
> Thanks Steve. FWIW, the reviewed-by tag has an incorrect email address:
>   Reviewed-by: Henrique Carvalho <[2]henrique.carvalho@suse.com>
> Should be
>   Reviewed-by: Henrique Carvalho <henrique.carvalho@suse.com>
>
> I'll close the corresponding PR at
> https://github.com/smfrench/smb3-utils/pull/16
>
> Cheers, David
>
> > On Mon, Mar 30, 2026 at 7:10 PM David Disseldorp <ddiss@suse.de> wrote:
> > >
> > > The existing (j > pass_length) check is insufficient to avoid dst buffer
> > > overrun into the start of the adjacent struct parsed_mount_info field.
> > > Check for overrun before writing to dst, and account for comma-expansion
> > > and null-termination.
> > >
> > > Bug: https://bugzilla.samba.org/show_bug.cgi?id=16044
> > >
> > > Reported-by: Bruno Bierbaumer <bruno@bierbaumer.net>
> > > Signed-off-by: David Disseldorp <ddiss@suse.de>
> > > ---
> > >  mount.cifs.c | 8 ++++----
> > >  1 file changed, 4 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/mount.cifs.c b/mount.cifs.c
> > > index 1923913..d41ca6a 100644
> > > --- a/mount.cifs.c
> > > +++ b/mount.cifs.c
> > > @@ -350,13 +350,13 @@ set_password(struct parsed_mount_info *parsed_info, const char *src,
> > >         unsigned int i = 0, j = 0;
> > >
> > >         while (src[i]) {
> > > -               if (src[i] == ',')
> > > -                       dst[j++] = ',';
> > > -               dst[j++] = src[i++];
> > > -               if (j > pass_length) {
> > > +               if (j + 2 >= pass_length) {
> > >                         fprintf(stderr, "Converted password too long!\n");
> > >                         return EX_USAGE;
> > >                 }
> > > +               if (src[i] == ',')
> > > +                       dst[j++] = ',';
> > > +               dst[j++] = src[i++];
> > >         }
> > >         dst[j] = '\0';
> > >         if (is_pass2)
> > > --
> > > 2.51.0
> > >
> >
> >
>


-- 
Thanks,

Steve

Re: [PATCH] mount.cifs: fix buffer overrun in set_password

[Enzo Matsumiya]

Hi Dave,

On 03/31, David Disseldorp wrote:
>The existing (j > pass_length) check is insufficient to avoid dst buffer
>overrun into the start of the adjacent struct parsed_mount_info field.
>Check for overrun before writing to dst, and account for comma-expansion
>and null-termination.
>
>Bug: https://bugzilla.samba.org/show_bug.cgi?id=16044
>
>Reported-by: Bruno Bierbaumer <bruno@bierbaumer.net>
>Signed-off-by: David Disseldorp <ddiss@suse.de>
>---
> mount.cifs.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
>diff --git a/mount.cifs.c b/mount.cifs.c
>index 1923913..d41ca6a 100644
>--- a/mount.cifs.c
>+++ b/mount.cifs.c
>@@ -350,13 +350,13 @@ set_password(struct parsed_mount_info *parsed_info, const char *src,
> 	unsigned int i = 0, j = 0;
>
> 	while (src[i]) {
>-		if (src[i] == ',')
>-			dst[j++] = ',';
>-		dst[j++] = src[i++];
>-		if (j > pass_length) {
>+		if (j + 2 >= pass_length) {

There is the overrun bug, yes, but unconditionally accounting for comma
expansion here will crop the password if it has at least 1 comma and
(unparsed) length == MOUNT_PASSWD_SIZE, e.g.:

(password here is <511 * 'a'> + ',')

# echo -e "username=administrator\npassword=$(printf 'a%.0s' {1..511})," > /tmp/creds
# ./mount.cifs -o credentials=/tmp/creds //w25.vm.test/test /mnt/test
Converted password too long!
error 1 (Operation not permitted) opening credential file /tmp/creds


Maybe password/password2 could be malloc'd with size
"strlen(src) + <number of commas in src>" and bounds checked against that?
Just a thought.


Cheers,

Enzo

Re: [PATCH] mount.cifs: fix buffer overrun in set_password

[Henrique Carvalho]

On Wed, Apr 01, 2026 at 10:40:41AM -0300, Enzo Matsumiya wrote:
> Hi Dave,
> 
> On 03/31, David Disseldorp wrote:
> > The existing (j > pass_length) check is insufficient to avoid dst buffer
> > overrun into the start of the adjacent struct parsed_mount_info field.
> > Check for overrun before writing to dst, and account for comma-expansion
> > and null-termination.
> > 
> > Bug: https://bugzilla.samba.org/show_bug.cgi?id=16044
> > 
> > Reported-by: Bruno Bierbaumer <bruno@bierbaumer.net>
> > Signed-off-by: David Disseldorp <ddiss@suse.de>
> > ---
> > mount.cifs.c | 8 ++++----
> > 1 file changed, 4 insertions(+), 4 deletions(-)
> > 
> > diff --git a/mount.cifs.c b/mount.cifs.c
> > index 1923913..d41ca6a 100644
> > --- a/mount.cifs.c
> > +++ b/mount.cifs.c
> > @@ -350,13 +350,13 @@ set_password(struct parsed_mount_info *parsed_info, const char *src,
> > 	unsigned int i = 0, j = 0;
> > 
> > 	while (src[i]) {
> > -		if (src[i] == ',')
> > -			dst[j++] = ',';
> > -		dst[j++] = src[i++];
> > -		if (j > pass_length) {
> > +		if (j + 2 >= pass_length) {
> 
> There is the overrun bug, yes, but unconditionally accounting for comma
> expansion here will crop the password if it has at least 1 comma and
> (unparsed) length == MOUNT_PASSWD_SIZE, e.g.:
> 
> (password here is <511 * 'a'> + ',')
> 
> # echo -e "username=administrator\npassword=$(printf 'a%.0s' {1..511})," > /tmp/creds
> # ./mount.cifs -o credentials=/tmp/creds //w25.vm.test/test /mnt/test
> Converted password too long!
> error 1 (Operation not permitted) opening credential file /tmp/creds
> 
> 
> Maybe password/password2 could be malloc'd with size
> "strlen(src) + <number of commas in src>" and bounds checked against that?
> Just a thought.
> 

Maybe we could have buffers double the size, but that would represent a
behavior change. Passwords containing commas were never guaranteed to
fit in current MOUNT_PASSWD_SIZE sized buffers.

I believe this would be a separate chage.

> 
> Cheers,
> 
> Enzo
> 

Re: [PATCH] mount.cifs: fix buffer overrun in set_password

[David Disseldorp]

Thanks for the feedback, Enzo and Henrique...

On Wed, 1 Apr 2026 12:24:37 -0300, Henrique Carvalho wrote:

> On Wed, Apr 01, 2026 at 10:40:41AM -0300, Enzo Matsumiya wrote:
> > Hi Dave,
> > 
> > On 03/31, David Disseldorp wrote:  
> > > The existing (j > pass_length) check is insufficient to avoid dst buffer
> > > overrun into the start of the adjacent struct parsed_mount_info field.
> > > Check for overrun before writing to dst, and account for comma-expansion
> > > and null-termination.
> > > 
> > > Bug: https://bugzilla.samba.org/show_bug.cgi?id=16044
> > > 
> > > Reported-by: Bruno Bierbaumer <bruno@bierbaumer.net>
> > > Signed-off-by: David Disseldorp <ddiss@suse.de>
> > > ---
> > > mount.cifs.c | 8 ++++----
> > > 1 file changed, 4 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/mount.cifs.c b/mount.cifs.c
> > > index 1923913..d41ca6a 100644
> > > --- a/mount.cifs.c
> > > +++ b/mount.cifs.c
> > > @@ -350,13 +350,13 @@ set_password(struct parsed_mount_info *parsed_info, const char *src,
> > > 	unsigned int i = 0, j = 0;
> > > 
> > > 	while (src[i]) {
> > > -		if (src[i] == ',')
> > > -			dst[j++] = ',';
> > > -		dst[j++] = src[i++];
> > > -		if (j > pass_length) {
> > > +		if (j + 2 >= pass_length) {  
> > 
> > There is the overrun bug, yes, but unconditionally accounting for comma
> > expansion here will crop the password if it has at least 1 comma and
> > (unparsed) length == MOUNT_PASSWD_SIZE, e.g.:
> > 
> > (password here is <511 * 'a'> + ',')
> > 
> > # echo -e "username=administrator\npassword=$(printf 'a%.0s' {1..511})," > /tmp/creds
> > # ./mount.cifs -o credentials=/tmp/creds //w25.vm.test/test /mnt/test
> > Converted password too long!
> > error 1 (Operation not permitted) opening credential file /tmp/creds

Yes, it's a valid point. I did consider it, but decided against it as:
- it makes the code slightly more complex, and adds more
  password-content based logic
- length restrictions are already opaque and confusing
  + "it broke when I changed password from 300x ':' to 300x ',' - why?"
- we're cropping the field any way (oversize pws were previously still
  passed to the kernel)

> > Maybe password/password2 could be malloc'd with size
> > "strlen(src) + <number of commas in src>" and bounds checked against that?
> > Just a thought.
> >   
> 
> Maybe we could have buffers double the size, but that would represent a
> behavior change. Passwords containing commas were never guaranteed to
> fit in current MOUNT_PASSWD_SIZE sized buffers.

These options would still leave us with password-content specific
behaviour, which seems fragile (open to side-channels attacks, etc).
Shouldn't the new mount API allow for the password to be passed though
to the kernel individually, and in-turn completely avoid comma escaping?

> I believe this would be a separate chage.

Agreed.