* [Ocfs2-devel] vfs: Add general support to enforce project quota limits
@ 2015-04-09 19:39 Dan Carpenter
2015-04-09 19:46 ` Dan Carpenter
2015-04-13 10:10 ` Jan Kara
0 siblings, 2 replies; 5+ messages in thread
From: Dan Carpenter @ 2015-04-09 19:39 UTC (permalink / raw)
To: ocfs2-devel
Hello Li Xi,
The patch 847aac644e92: "vfs: Add general support to enforce project
quota limits" from Mar 19, 2015, leads to the following static
checker warning:
fs/ocfs2/quota_local.c:183 ocfs2_local_check_quota_file()
error: buffer overflow 'lmagics' 2 <= 2
fs/ocfs2/quota_local.c
159 /* Check whether we understand format of quota files */
160 static int ocfs2_local_check_quota_file(struct super_block *sb, int type)
161 {
162 unsigned int lmagics[OCFS2_MAXQUOTAS] = OCFS2_LOCAL_QMAGICS;
^^^^^^^^^^^^^^^
This is 2. Maybe the fix is to change this to MAXQUOTAS.
163 unsigned int lversions[OCFS2_MAXQUOTAS] = OCFS2_LOCAL_QVERSIONS;
164 unsigned int gmagics[OCFS2_MAXQUOTAS] = OCFS2_GLOBAL_QMAGICS;
165 unsigned int gversions[OCFS2_MAXQUOTAS] = OCFS2_GLOBAL_QVERSIONS;
166 unsigned int ino[OCFS2_MAXQUOTAS] = { USER_QUOTA_SYSTEM_INODE,
167 GROUP_QUOTA_SYSTEM_INODE };
168 struct buffer_head *bh = NULL;
169 struct inode *linode = sb_dqopt(sb)->files[type];
170 struct inode *ginode = NULL;
171 struct ocfs2_disk_dqheader *dqhead;
172 int status, ret = 0;
173
174 /* First check whether we understand local quota file */
175 status = ocfs2_read_quota_block(linode, 0, &bh);
176 if (status) {
177 mlog_errno(status);
178 mlog(ML_ERROR, "failed to read quota file header (type=%d)\n",
179 type);
180 goto out_err;
181 }
182 dqhead = (struct ocfs2_disk_dqheader *)(bh->b_data);
183 if (le32_to_cpu(dqhead->dqh_magic) != lmagics[type]) {
^^^^^^^^^^^^^
This is one past the end of the array. It used to be limitied in
do_quotactl().
if (type >= (XQM_COMMAND(cmd) ? XQM_MAXQUOTAS : MAXQUOTAS))
The old logic was the XFS had 3 quotas and everyone else had 2 but now
we raised MAXQUOTAS to 3 as well.
184 mlog(ML_ERROR, "quota file magic does not match (%u != %u),"
185 " type=%d\n", le32_to_cpu(dqhead->dqh_magic),
186 lmagics[type], type);
187 goto out_err;
188 }
regards,
dan carpenter
^ permalink raw reply [flat|nested] 5+ messages in thread
* [Ocfs2-devel] vfs: Add general support to enforce project quota limits
2015-04-09 19:39 [Ocfs2-devel] vfs: Add general support to enforce project quota limits Dan Carpenter
@ 2015-04-09 19:46 ` Dan Carpenter
2015-04-13 10:10 ` Jan Kara
1 sibling, 0 replies; 5+ messages in thread
From: Dan Carpenter @ 2015-04-09 19:46 UTC (permalink / raw)
To: ocfs2-devel
Here is a related warning. I haven't investigated it.
fs/quota/quota_v1.c:149 v1_check_quota_file() error: buffer overflow 'quota_magics' 2 <= 2
regards,
dan carpenter
^ permalink raw reply [flat|nested] 5+ messages in thread
* [Ocfs2-devel] vfs: Add general support to enforce project quota limits
2015-04-09 19:39 [Ocfs2-devel] vfs: Add general support to enforce project quota limits Dan Carpenter
2015-04-09 19:46 ` Dan Carpenter
@ 2015-04-13 10:10 ` Jan Kara
2015-04-13 11:48 ` Dan Carpenter
1 sibling, 1 reply; 5+ messages in thread
From: Jan Kara @ 2015-04-13 10:10 UTC (permalink / raw)
To: ocfs2-devel
Hello Dan,
On Thu 09-04-15 22:39:54, Dan Carpenter wrote:
> The patch 847aac644e92: "vfs: Add general support to enforce project
> quota limits" from Mar 19, 2015, leads to the following static
> checker warning:
Thanks for letting us know.
> fs/ocfs2/quota_local.c:183 ocfs2_local_check_quota_file()
> error: buffer overflow 'lmagics' 2 <= 2
OK, so the checker thinks that 'type' argument can be larger than
OCFS2_MAXQUOTAS.
> fs/ocfs2/quota_local.c
> 159 /* Check whether we understand format of quota files */
> 160 static int ocfs2_local_check_quota_file(struct super_block *sb, int type)
> 161 {
> 162 unsigned int lmagics[OCFS2_MAXQUOTAS] = OCFS2_LOCAL_QMAGICS;
> ^^^^^^^^^^^^^^^
> This is 2. Maybe the fix is to change this to MAXQUOTAS.
No, that isn't the right fix. OCFS2 still supports only two quota types.
The right fix is to make sure that type == 2 cannot reach filesystems which
don't support it. See below...
> 163 unsigned int lversions[OCFS2_MAXQUOTAS] = OCFS2_LOCAL_QVERSIONS;
> 164 unsigned int gmagics[OCFS2_MAXQUOTAS] = OCFS2_GLOBAL_QMAGICS;
> 165 unsigned int gversions[OCFS2_MAXQUOTAS] = OCFS2_GLOBAL_QVERSIONS;
> 166 unsigned int ino[OCFS2_MAXQUOTAS] = { USER_QUOTA_SYSTEM_INODE,
> 167 GROUP_QUOTA_SYSTEM_INODE };
> 168 struct buffer_head *bh = NULL;
> 169 struct inode *linode = sb_dqopt(sb)->files[type];
> 170 struct inode *ginode = NULL;
> 171 struct ocfs2_disk_dqheader *dqhead;
> 172 int status, ret = 0;
> 173
> 174 /* First check whether we understand local quota file */
> 175 status = ocfs2_read_quota_block(linode, 0, &bh);
> 176 if (status) {
> 177 mlog_errno(status);
> 178 mlog(ML_ERROR, "failed to read quota file header (type=%d)\n",
> 179 type);
> 180 goto out_err;
> 181 }
> 182 dqhead = (struct ocfs2_disk_dqheader *)(bh->b_data);
> 183 if (le32_to_cpu(dqhead->dqh_magic) != lmagics[type]) {
> ^^^^^^^^^^^^^
> This is one past the end of the array. It used to be limitied in
> do_quotactl().
>
> if (type >= (XQM_COMMAND(cmd) ? XQM_MAXQUOTAS : MAXQUOTAS))
>
> The old logic was the XFS had 3 quotas and everyone else had 2 but now
> we raised MAXQUOTAS to 3 as well.
Yes, but we have there a test:
if (!(sb->s_quota_types & (1 << type)))
return -EINVAL;
which also limits allowed types. And OCFS2 sets this to QTYPE_MASK_USR |
QTYPE_MASK_GRP so type == 2 cannot reach it via quotactl(). So either this
is a false positive or there's some other path how type == 2 can reach
OCFS2 which I'm missing and I'd definitely like to learn about it. Can you
investigate a bit please? Thanks.
Honza
--
Jan Kara <jack@suse.cz>
SUSE Labs, CR
^ permalink raw reply [flat|nested] 5+ messages in thread
* [Ocfs2-devel] vfs: Add general support to enforce project quota limits
2015-04-13 10:10 ` Jan Kara
@ 2015-04-13 11:48 ` Dan Carpenter
2015-04-13 12:19 ` Jan Kara
0 siblings, 1 reply; 5+ messages in thread
From: Dan Carpenter @ 2015-04-13 11:48 UTC (permalink / raw)
To: ocfs2-devel
Ah. Thanks for looking into it. Smatch can't understand bit flags like
that yet. And it also still doesn't track pointers very well.
Eventually those things will be improved.
I see now that this out of bounds is also prevented in
vfs_load_quota_inode(). We don't allow "(type == PRJQUOTA &&
sb->dq_op->get_projid == NULL)". Since, ->get_projid() is always NULL
for OCFS2 that means "type" can't equal to PRJQUOTA (2).
Hm.. is ->get_projid set anywhere? Cscope and smatch can't find it if
it is.
I haven't investigated v1_read_file_info() but presumably it's the same.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 5+ messages in thread
* [Ocfs2-devel] vfs: Add general support to enforce project quota limits
2015-04-13 11:48 ` Dan Carpenter
@ 2015-04-13 12:19 ` Jan Kara
0 siblings, 0 replies; 5+ messages in thread
From: Jan Kara @ 2015-04-13 12:19 UTC (permalink / raw)
To: ocfs2-devel
On Mon 13-04-15 14:48:51, Dan Carpenter wrote:
> Ah. Thanks for looking into it. Smatch can't understand bit flags like
> that yet. And it also still doesn't track pointers very well.
> Eventually those things will be improved.
>
> I see now that this out of bounds is also prevented in
> vfs_load_quota_inode(). We don't allow "(type == PRJQUOTA &&
> sb->dq_op->get_projid == NULL)". Since, ->get_projid() is always NULL
> for OCFS2 that means "type" can't equal to PRJQUOTA (2).
>
> Hm.. is ->get_projid set anywhere? Cscope and smatch can't find it if
> it is.
Not yet. There are ext4 patches floating around that will use this.
Honza
--
Jan Kara <jack@suse.cz>
SUSE Labs, CR
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2015-04-13 12:19 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-04-09 19:39 [Ocfs2-devel] vfs: Add general support to enforce project quota limits Dan Carpenter
2015-04-09 19:46 ` Dan Carpenter
2015-04-13 10:10 ` Jan Kara
2015-04-13 11:48 ` Dan Carpenter
2015-04-13 12:19 ` Jan Kara
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.