[bug report] fs/ntfs3: Add attrib operations

* [bug report] fs/ntfs3: Add attrib operations
@ 2021-08-24  9:53 Dan Carpenter
  0 siblings, 0 replies; 4+ messages in thread
From: Dan Carpenter @ 2021-08-24  9:53 UTC (permalink / raw)
  To: almaz.alexandrovich; +Cc: ntfs3

Hello Konstantin Komarov,

The patch be71b5cba2e6: "fs/ntfs3: Add attrib operations" from Aug
13, 2021, leads to the following
Smatch static checker warning:

	fs/ntfs3/attrib.c:882 attr_data_get_block()
	warn: was expecting a 64 bit value instead of '~(clst_per_frame - 1)'

fs/ntfs3/attrib.c
    823 int attr_data_get_block(struct ntfs_inode *ni, CLST vcn, CLST clen, CLST *lcn,
    824 			CLST *len, bool *new)
    825 {
    826 	int err = 0;
    827 	struct runs_tree *run = &ni->file.run;
    828 	struct ntfs_sb_info *sbi;
    829 	u8 cluster_bits;
    830 	struct ATTRIB *attr = NULL, *attr_b;
    831 	struct ATTR_LIST_ENTRY *le, *le_b;
    832 	struct mft_inode *mi, *mi_b;
    833 	CLST hint, svcn, to_alloc, evcn1, next_svcn, asize, end;
    834 	u64 total_size;
    835 	u32 clst_per_frame;
    836 	bool ok;
    837 
    838 	if (new)
    839 		*new = false;
    840 
    841 	down_read(&ni->file.run_lock);
    842 	ok = run_lookup_entry(run, vcn, lcn, len, NULL);
    843 	up_read(&ni->file.run_lock);
    844 
    845 	if (ok && (*lcn != SPARSE_LCN || !new)) {
    846 		/* normal way */
    847 		return 0;
    848 	}
    849 
    850 	if (!clen)
    851 		clen = 1;
    852 
    853 	if (ok && clen > *len)
    854 		clen = *len;
    855 
    856 	sbi = ni->mi.sbi;
    857 	cluster_bits = sbi->cluster_bits;
    858 
    859 	ni_lock(ni);
    860 	down_write(&ni->file.run_lock);
    861 
    862 	le_b = NULL;
    863 	attr_b = ni_find_attr(ni, NULL, &le_b, ATTR_DATA, NULL, 0, NULL, &mi_b);
    864 	if (!attr_b) {
    865 		err = -ENOENT;
    866 		goto out;
    867 	}
    868 
    869 	if (!attr_b->non_res) {
    870 		*lcn = RESIDENT_LCN;
    871 		*len = 1;
    872 		goto out;
    873 	}
    874 
    875 	asize = le64_to_cpu(attr_b->nres.alloc_size) >> sbi->cluster_bits;
    876 	if (vcn >= asize) {
    877 		err = -EINVAL;
    878 		goto out;
    879 	}
    880 
    881 	clst_per_frame = 1u << attr_b->nres.c_unit;
--> 882 	to_alloc = (clen + clst_per_frame - 1) & ~(clst_per_frame - 1);

In this config "clen" is a u64 and "clst_per_frame" is a u32 so this
code will truncate to_alloc to a u32.  An easy fix is to use the
ALIGN() macro.

		to_alloc = ALIGN(clen, clst_per_frame);

However, I still had some questions below so I did not write a patch.

    883 
    884 	if (vcn + to_alloc > asize)
    885 		to_alloc = asize - vcn;

If to_alloc is too large for asize then make it smaller.  Is it still
ALIGNED?

    886 
    887 	svcn = le64_to_cpu(attr_b->nres.svcn);
    888 	evcn1 = le64_to_cpu(attr_b->nres.evcn) + 1;
    889 
    890 	attr = attr_b;
    891 	le = le_b;
    892 	mi = mi_b;
    893 
    894 	if (le_b && (vcn < svcn || evcn1 <= vcn)) {
    895 		attr = ni_find_attr(ni, attr_b, &le, ATTR_DATA, NULL, 0, &vcn,
    896 				    &mi);
    897 		if (!attr) {
    898 			err = -EINVAL;
    899 			goto out;
    900 		}
    901 		svcn = le64_to_cpu(attr->nres.svcn);
    902 		evcn1 = le64_to_cpu(attr->nres.evcn) + 1;
    903 	}
    904 
    905 	err = attr_load_runs(attr, ni, run, NULL);
    906 	if (err)
    907 		goto out;
    908 
    909 	if (!ok) {
    910 		ok = run_lookup_entry(run, vcn, lcn, len, NULL);
    911 		if (ok && (*lcn != SPARSE_LCN || !new)) {
    912 			/* normal way */
    913 			err = 0;
    914 			goto ok;
    915 		}
    916 
    917 		if (!ok && !new) {
    918 			*len = 0;
    919 			err = 0;
    920 			goto ok;
    921 		}
    922 
    923 		if (ok && clen > *len) {
    924 			clen = *len;
    925 			to_alloc = (clen + clst_per_frame - 1) &
    926 				   ~(clst_per_frame - 1);

We re-assign to_alloc here.  And it's smaller than before, but it hasn't
been checked against asize so it might not be small enough?

    927 		}
    928 	}
    929 
    930 	if (!is_attr_ext(attr_b)) {
    931 		err = -EINVAL;
    932 		goto out;
    933 	}
    934 

regards,
dan carpenter

^ permalink raw reply	[flat|nested] 4+ messages in thread