Re: [PATCH] cifs: Convert struct fealist away from 1-element array

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Kees,

On Fri, Feb 09, 2024 at 04:02:32PM -0800, Kees Cook wrote:
> On Sat, Feb 10, 2024 at 01:13:06AM +0300, Vitaly Chikunov wrote:
> > On Tue, Feb 14, 2023 at 04:08:39PM -0800, Kees Cook wrote:
> > > The kernel is globally removing the ambiguous 0-length and 1-element
> > > arrays in favor of flexible arrays, so that we can gain both compile-time
> > > and run-time array bounds checking[1].
> > > 
> > > While struct fealist is defined as a "fake" flexible array (via a
> > > 1-element array), it is only used for examination of the first array
> > > element. Walking the list is performed separately, so there is no reason
> > > to treat the "list" member of struct fealist as anything other than a
> > > single entry. Adjust the struct and code to match.
> > > 
> > > Additionally, struct fea uses the "name" member either as a dynamic
> > > string, or is manually calculated from the start of the struct. Redefine
> > > the member as a flexible array.
> > > 
> > > No machine code output differences are produced after these changes.
> > > 
> > > [1] For lots of details, see both:
> > >     https://docs.kernel.org/process/deprecated.html#zero-length-and-one-element-arrays
> > >     https://people.kernel.org/kees/bounded-flexible-arrays-in-c
> > > 
> > > Cc: Steve French <sfrench@xxxxxxxxx>
> > > Cc: Paulo Alcantara <pc@xxxxxx>
> > > Cc: Ronnie Sahlberg <lsahlber@xxxxxxxxxx>
> > > Cc: Shyam Prasad N <sprasad@xxxxxxxxxxxxx>
> > > Cc: Tom Talpey <tom@xxxxxxxxxx>
> > > Cc: linux-cifs@xxxxxxxxxxxxxxx
> > > Cc: samba-technical@xxxxxxxxxxxxxxx
> > > Signed-off-by: Kees Cook <keescook@xxxxxxxxxxxx>
> > > ---
> > >  fs/cifs/cifspdu.h |  4 ++--
> > >  fs/cifs/cifssmb.c | 16 ++++++++--------
> > >  2 files changed, 10 insertions(+), 10 deletions(-)
> > > 
> > > diff --git a/fs/cifs/cifspdu.h b/fs/cifs/cifspdu.h
> > > index 623caece2b10..add73be4902c 100644
> > > --- a/fs/cifs/cifspdu.h
> > > +++ b/fs/cifs/cifspdu.h
> > > @@ -2583,7 +2583,7 @@ struct fea {
> > >  	unsigned char EA_flags;
> > >  	__u8 name_len;
> > >  	__le16 value_len;
> > > -	char name[1];
> > > +	char name[];
> > >  	/* optionally followed by value */
> > >  } __attribute__((packed));
> > >  /* flags for _FEA.fEA */
> > > @@ -2591,7 +2591,7 @@ struct fea {
> > >  
> > >  struct fealist {
> > >  	__le32 list_len;
> > > -	struct fea list[1];
> > > +	struct fea list;
> > >  } __attribute__((packed));
> > >  
> > >  /* used to hold an arbitrary blob of data */
> > > diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c
> > > index 60dd4e37030a..7c587157d030 100644
> > > --- a/fs/cifs/cifssmb.c
> > > +++ b/fs/cifs/cifssmb.c
> > > @@ -5787,7 +5787,7 @@ CIFSSMBQAllEAs(const unsigned int xid, struct cifs_tcon *tcon,
> > >  
> > >  	/* account for ea list len */
> > >  	list_len -= 4;
> > > -	temp_fea = ea_response_data->list;
> > > +	temp_fea = &ea_response_data->list;
> > >  	temp_ptr = (char *)temp_fea;
> > >  	while (list_len > 0) {
> > >  		unsigned int name_len;
> > > @@ -5902,7 +5902,7 @@ CIFSSMBSetEA(const unsigned int xid, struct cifs_tcon *tcon,
> > >  	else
> > >  		name_len = strnlen(ea_name, 255);
> > >  
> > > -	count = sizeof(*parm_data) + ea_value_len + name_len;
> > > +	count = sizeof(*parm_data) + 1 + ea_value_len + name_len;
> > >  	pSMB->MaxParameterCount = cpu_to_le16(2);
> > >  	/* BB find max SMB PDU from sess */
> > >  	pSMB->MaxDataCount = cpu_to_le16(1000);
> > > @@ -5926,14 +5926,14 @@ CIFSSMBSetEA(const unsigned int xid, struct cifs_tcon *tcon,
> > >  	byte_count = 3 /* pad */  + params + count;
> > >  	pSMB->DataCount = cpu_to_le16(count);
> > >  	parm_data->list_len = cpu_to_le32(count);
> > > -	parm_data->list[0].EA_flags = 0;
> > > +	parm_data->list.EA_flags = 0;
> > >  	/* we checked above that name len is less than 255 */
> > > -	parm_data->list[0].name_len = (__u8)name_len;
> > > +	parm_data->list.name_len = (__u8)name_len;
> > >  	/* EA names are always ASCII */
> > >  	if (ea_name)
> > > -		strncpy(parm_data->list[0].name, ea_name, name_len);
> > > -	parm_data->list[0].name[name_len] = 0;
> > > -	parm_data->list[0].value_len = cpu_to_le16(ea_value_len);
> > > +		strncpy(parm_data->list.name, ea_name, name_len);
> > 
> > Could non-applying this patch cause false-positive fortify_panic?
> > We got a bug report from user of 6.1.73:
> > 
> >    Jan 24 15:15:20 kalt2test.dpt.local kernel: detected buffer overflow in strncpy
> 
> Yes, this seems likely. I would backport this change.
> 
> >    Jan 24 15:15:20 kalt2test.dpt.local kernel: ------------[ cut here ]------------
> >    Jan 24 15:15:20 kalt2test.dpt.local kernel: kernel BUG at lib/string_helpers.c:1027!
> >    ...
> >    Jan 24 15:15:20 kalt2test.dpt.local kernel: Call Trace:
> >    Jan 24 15:15:20 kalt2test.dpt.local kernel:  CIFSSMBSetEA.cold+0xc/0x18 [cifs]
> >    Jan 24 15:15:20 kalt2test.dpt.local kernel:  cifs_xattr_set+0x596/0x690 [cifs]
> >    Jan 24 15:15:20 kalt2test.dpt.local kernel:  __vfs_removexattr+0x52/0x70
> >    Jan 24 15:15:20 kalt2test.dpt.local kernel:  __vfs_removexattr_locked+0xbc/0x150
> >    Jan 24 15:15:20 kalt2test.dpt.local kernel:  vfs_removexattr+0x56/0x100
> >    Jan 24 15:15:20 kalt2test.dpt.local kernel:  removexattr+0x58/0x90
> >    Jan 24 15:15:20 kalt2test.dpt.local kernel:  __ia32_sys_fremovexattr+0x80/0xa0
> >    Jan 24 15:15:20 kalt2test.dpt.local kernel:  int80_emulation+0xa9/0x110
> >    Jan 24 15:15:20 kalt2test.dpt.local kernel:  asm_int80_emulation+0x16/0x20
> 
> This appears to be a compat call?

Likely. This is caused through Wine.

Thanks,

> 
> -Kees
> 
> > 
> > I don't find this patch appled to stable/linux-6.1.y.
> > 
> > Thanks,
> > 
> > ps. (Unfortunately `CIFSSMBSetEA+0xc` address is not resolvable to the
> > actual line inside of CIFSSMBSetEA pointing just to the head of it.
> > 
> >    (gdb) l *CIFSSMBSetEA+0xc
> >    0x6de3c is in CIFSSMBSetEA (fs/smb/client/cifssmb.c:5776).
> >    5771    int
> >    5772    CIFSSMBSetEA(const unsigned int xid, struct cifs_tcon *tcon,
> >    5773                 const char *fileName, const char *ea_name, const void *ea_value,
> >    5774                 const __u16 ea_value_len, const struct nls_table *nls_codepage,
> >    5775                 struct cifs_sb_info *cifs_sb)
> >    5776    {
> >    5777            struct smb_com_transaction2_spi_req *pSMB = NULL;
> >    5778            struct smb_com_transaction2_spi_rsp *pSMBr = NULL;
> >    5779            struct fealist *parm_data;
> >    5780            int name_len;
> > 
> > But there is only one strncpy there.
> > 
> > > +	parm_data->list.name[name_len] = '\0';
> > > +	parm_data->list.value_len = cpu_to_le16(ea_value_len);
> > >  	/* caller ensures that ea_value_len is less than 64K but
> > >  	we need to ensure that it fits within the smb */
> > >  
> > > @@ -5941,7 +5941,7 @@ CIFSSMBSetEA(const unsigned int xid, struct cifs_tcon *tcon,
> > >  	     negotiated SMB buffer size BB */
> > >  	/* if (ea_value_len > buffer_size - 512 (enough for header)) */
> > >  	if (ea_value_len)
> > > -		memcpy(parm_data->list[0].name+name_len+1,
> > > +		memcpy(parm_data->list.name + name_len + 1,
> > >  		       ea_value, ea_value_len);
> > >  
> > >  	pSMB->TotalDataCount = pSMB->DataCount;
> > > -- 
> > > 2.34.1
> > > 
> 
> -- 
> Kees Cook




[Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux